Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Isis Pagination update #18357

Merged
merged 5 commits into from
Oct 26, 2017
Merged

Isis Pagination update #18357

merged 5 commits into from
Oct 26, 2017

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Oct 17, 2017 •
edited by joomla-cms-bot
Loading

Improve the accessibility of the pagination - there are no visual changes

  1. Wrap the pagination in a nav and add a role (needed for IE) and an aria-label
  2. Ensure font icons have aria-hidden=true
  3. Add aria-current to the selected page
  4. Add aria-title to the active links eg Go to page 1

Note 1

This PR does not include the language strings as they were added in #18326

Note 2

I dont know why but isis and protostar (see #18326) use an override for pagination instead of the layout. For b/c this PR is just for the override but when approved I will do a similar PR for the layout

Thanks to @fuzzbomb (Drupal 8 Core Accessibility Maintainer) for his advice and reviewing this

@brianteeman
go to page and current page
eeef58e 
aria-label start, previous etc
aria-hidden=true
add nav
@brianteeman brianteeman mentioned this pull request Oct 17, 2017
Merged
Quy
Quy reviewed Oct 17, 2017
View reviewed changes
administrator/templates/isis/html/layouts/joomla/pagination/link.php Outdated
break;
}

$item->text .= $addText ?: '';

if ($icon !== null)
{
$display = '<span class="' . $icon . '"></span>';
$display = '<span class="' . $icon . ' aria-hidden="true"></span>';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add closing " to the class attribute.

Quy
Quy reviewed Oct 17, 2017
View reviewed changes
administrator/templates/isis/html/layouts/joomla/pagination/link.php Outdated
@@ -95,12 +100,14 @@
?>
<?php if ($displayData['active']) : ?>
<li<?php echo $liClass ? ' class="' . $liClass . '"' : ''; ?>>
<a <?php echo $cssClasses ? 'class="' . implode(' ', $cssClasses) . '"' : ''; ?> <?php echo $title; ?> href="#" onclick="<?php echo $onClick; ?>">
<a aria-label="<?php echo $aria;?>" <?php echo $cssClasses ? 'class="' . implode(' ', $cssClasses) . '"' : ''; ?> <?php echo $title; ?> href="#" onclick="<?php echo $onClick; ?>">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add space after $aria;

@Quy
Copy link
Contributor

Quy commented Oct 17, 2017 •
edited
Loading

See output. Page Previous shouldn't be aria-current="true" right?? Can you have more than one aria-current="true"?

			<nav role="navigation" aria-label="Pagination">
			<ul class="pagination-list">
					<li class="disabled">
		<span aria-current="true" aria-label="Page Start (Page 1 of 4)">
			<span class="icon-backward icon-first" aria-hidden="true"></span>		</span>
	</li>
	<li class="disabled">
		<span aria-current="true" aria-label="Page Previous">
			<span class="icon-step-backward icon-previous" aria-hidden="true"></span>		</span>
	</li>
										<li class="active">
		<span aria-current="true" aria-label="Page 1">
			1		</span>
	</li>

@brianteeman
Copy link
Contributor Author

no that looks wrong - i will check it again later

@brianteeman
Copy link
Contributor Author

@Quy open to suggestions/help - the code here is a bit funky

Quy
Quy reviewed Oct 17, 2017
View reviewed changes
administrator/templates/isis/html/layouts/joomla/pagination/link.php Outdated
<?php echo $display; ?>
</a>
</li>
<?php else : ?>
<li class="<?php echo $class; ?>">
<span><?php echo $display; ?></span>
<span aria-current="true" aria-label="<?php echo JText::sprintf('JLIB_HTML_PAGE_CURRENT', $item->text); ?>">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should do the trick. Please test.

<span <?php echo $class == 'active' ? 'aria-current="true"' : '' ?> aria-label="<?php echo JText::sprintf('JLIB_HTML_PAGE_CURRENT', $item->text); ?>">

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats similar to one of the things i tried - but it doesnt work as it produces

<li class="disabled">
		<span aria-label="JLIB_HTML_PAGE_CURRENT">
			<span class="icon-step-backward icon-previous" aria-hidden="true"></span>	
		</span>
</li>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok should be fixed now - thanks

Quy
Quy reviewed Oct 17, 2017
View reviewed changes
administrator/templates/isis/html/layouts/joomla/pagination/link.php Outdated
@@ -95,12 +100,14 @@
?>
<?php if ($displayData['active']) : ?>
<li<?php echo $liClass ? ' class="' . $liClass . '"' : ''; ?>>
<a <?php echo $cssClasses ? 'class="' . implode(' ', $cssClasses) . '"' : ''; ?> <?php echo $title; ?> href="#" onclick="<?php echo $onClick; ?>">
<a aria-label="<?php echo $aria ;?>" <?php echo $cssClasses ? 'class="' . implode(' ', $cssClasses) . '"' : ''; ?> <?php echo $title; ?> href="#" onclick="<?php echo $onClick; ?>">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the space after ;

Quy
Quy reviewed Oct 18, 2017
View reviewed changes
administrator/templates/isis/html/layouts/joomla/pagination/link.php Outdated
<?php echo $display; ?>
</a>
</li>
<?php else : ?>
<li class="<?php echo $class; ?>">
<span><?php echo $display; ?></span>
<span <?php echo $class == 'active' ? 'aria-current="true" aria-label="' . JText::sprintf('JLIB_HTML_PAGE_CURRENT', $item->text) . '"' : '' ?>>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add semicolon after : ''

@Quy
Copy link
Contributor

Quy commented Oct 18, 2017

I have tested this item ✅ successfully on 94bb83c

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18357.

@ghost
Copy link

ghost commented Oct 25, 2017

@Quy can you please explain how the Code looks without and with Pull Request?

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18357.

@Quy
Copy link
Contributor

Quy commented Oct 25, 2017

Go to Content > Articles
View page source
Find <ul class="pagination-list">

Before PR:
pagination-before

After PR:
pagination

@ghost
Copy link

ghost commented Oct 25, 2017

I have tested this item ✅ successfully on 94bb83c

Thanks for Help, @Quy

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18357.

@ghost
Copy link

ghost commented Oct 25, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 25, 2017
@mbabker mbabker merged commit be10e5c into joomla:staging Oct 26, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 26, 2017
@mbabker mbabker added this to the Joomla 3.8.2 milestone Oct 26, 2017
@brianteeman
Copy link
Contributor Author

Thanks

@brianteeman brianteeman deleted the isis-pagination branch October 26, 2017 00:08
@brianteeman brianteeman mentioned this pull request Jul 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Quy Quy Quy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Joomla 3.8.2
Development

Successfully merging this pull request may close these issues.
4 participants
@brianteeman @Quy @mbabker @joomla-cms-bot