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.

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

breaking(Pagination): rename ariaLabel prop aria-label #2607

Merged
merged 8 commits into from
Mar 18, 2018

Conversation

AndreiEnache
Copy link
Contributor

@AndreiEnache AndreiEnache commented Mar 5, 2018

BREAKING CHANGE

This PR renames ariaLabel prop that used in Pagination component to the native aria-label prop.

Before

<Pagination ariaLabel='A label' />
<Pagination prevItem={{ ariaLabel: 'A label' }} />
<Pagination.Item ariaLabel='A label' />

After

<Pagination aria-label='A label' />
<Pagination prevItem={{ 'aria-label': 'A label' }} />
<Pagination.Item aria-label='A label' />

Fixes #2601.

It also requires update to latest version of babel-plugin-transform-react-handled-props because previos version have problems with parse of string properties.

@welcome
Copy link

welcome bot commented Mar 5, 2018

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Run yarn lint locally to catch formatting errors. This will fix some errors automatically, commit and push any changes.
  • Run yarn test locally to catch errors. This ensures all components still behave as they should.
  • Run yarn start to run the doc site locally and try a few pages, ensuring everything is in good working order.
  • Include tests when adding/changing behavior.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@codecov-io
Copy link

codecov-io commented Mar 5, 2018

Codecov Report

Merging #2607 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2607      +/-   ##
==========================================
- Coverage   99.74%   99.74%   -0.01%     
==========================================
  Files         160      160              
  Lines        2751     2748       -3     
==========================================
- Hits         2744     2741       -3     
  Misses          7        7
Impacted Files Coverage Δ
src/addons/Pagination/PaginationItem.js 100% <100%> (ø) ⬆️
src/addons/Pagination/Pagination.js 100% <100%> (ø) ⬆️
src/collections/Menu/MenuItem.js 100% <100%> (ø) ⬆️
src/behaviors/Visibility/Visibility.js 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36221c0...2c169fe. Read the comment docs.

@layershifter
Copy link
Member

Let's modify Pagination component, it should use aria-label instead of ariaLabel.

@AndreiEnache
Copy link
Contributor Author

AndreiEnache commented Mar 6, 2018

I might be wrong, but the problem is not with the Pagination component.
The pagination component uses aria-label instead of ariaLabel here:
Taken from Pagination.js line 138

<Menu {...rest} aria-label={ariaLabel} pagination role='navigation'>

While going further down the road, I saw that PaginationItems are created. Looking into PaginationItem.js, I saw that it actually creates a modified MenuItem
Taken from PaginationItem.js line 79

MenuItem.create(this.props, {
      defaultProps: {
        active,
        disabled,
        'aria-current': active,
        'aria-label': ariaLabel,

So the PaginationItem creates a MenuItem and passes props to it.
In MenuItem, I saw that the unhandled props are taken const rest = getUnhandledProps(MenuItem, this.props) and spread as props to the element
Taken from MenuItem.js line 134/141

<ElementType {...rest} className={classes} onClick={this.handleClick}>

After modifying the above line to handle ariaLabel as a special prop and manually inserting it into the element as aria-label (as per react docs), the github pages build stopped giving the warning.

Again, I might be wrong, it's my first pull request, but I did try to understand what happens.

@layershifter layershifter changed the title fix(MenuItem): Invalid ARIA attribute ariaLabel breaking(Pagination): rename ariaLabel prop aria-label Mar 11, 2018
@layershifter
Copy link
Member

@AndreiEnache in fact, problem is with-in Pagination component. It uses ariaLabel prop, but it should use the native aria-label, as we downpass all unhandled props ariaLabel will be passed to Menu.Item.

@AndreiEnache
Copy link
Contributor Author

@layershifter You're right!
Looking through your commit now I understand what the actual issue was.
I identified the fact that unhandled props are being spread into the component but I thought it could be handled into the component in which they are passed rather than in the pagination; your approach is indeed much cleaner.

@levithomason levithomason merged commit ce3586e into Semantic-Org:master Mar 18, 2018
@welcome
Copy link

welcome bot commented Mar 18, 2018

Congrats on merging your first pull request! 🎉🎉🎉

robot victory dance

@levithomason
Copy link
Member

Released in [email protected].

layershifter pushed a commit that referenced this pull request Mar 20, 2018
* fix(MenuItem): Invalid ARIA attribute ariaLabel

* Fixes trailing whitespace

* Fixes line length

* fix(Pagination): update usage of 'aria-label' prop

* fix(Pagination): update usage of 'aria-label' prop
HansCronau added a commit to HansCronau/DemoBlog that referenced this pull request Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants