Skip to content

Isis Pagination update#18357

Merged
mbabker merged 5 commits intojoomla:stagingfrom
brianteeman:isis-pagination
Oct 26, 2017
Merged

Isis Pagination update#18357
mbabker merged 5 commits intojoomla:stagingfrom
brianteeman:isis-pagination

Conversation

@brianteeman
Copy link
Copy Markdown
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
if ($icon !== null)
{
$display = '<span class="' . $icon . '"></span>';
$display = '<span class="' . $icon . ' aria-hidden="true"></span>';
Copy link
Copy Markdown
Contributor

@Quy Quy Oct 17, 2017

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
<?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
Copy Markdown
Contributor

@Quy Quy Oct 17, 2017

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
Copy Markdown
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
Copy Markdown
Contributor Author

brianteeman commented Oct 17, 2017

no that looks wrong - i will check it again later

@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Oct 17, 2017

@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 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
Copy Markdown
Contributor

@Quy Quy Oct 17, 2017

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
Copy Markdown
Contributor Author

@brianteeman brianteeman Oct 17, 2017

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
Copy Markdown
Contributor Author

@brianteeman brianteeman Oct 17, 2017

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
<?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
Copy Markdown
Contributor

@Quy Quy Oct 17, 2017

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 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
Copy Markdown
Contributor

@Quy Quy Oct 18, 2017

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

ghost commented Oct 25, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added RTC This Pull Request is Ready To Commit and removed RTC This Pull Request is Ready To Commit labels Oct 25, 2017
@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Oct 26, 2017

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