Skip to content

fix(login page): removes ariaLabel from checkbox #2455

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
Merged
tlabaj merged 5 commits into from
Jul 11, 2019
Merged

fix(login page): removes ariaLabel from checkbox #2455

tlabaj merged 5 commits into from
Jul 11, 2019

Conversation

@jenny-s51
Copy link
Contributor

@jenny-s51 jenny-s51 commented Jul 3, 2019

closes #1737

@jenny-s51 jenny-s51 requested review from dlabaj, jgiardino and tlabaj July 3, 2019 17:23
@patternfly-build
Copy link
Collaborator

patternfly-build commented Jul 3, 2019

PatternFly-React preview: https://patternfly-react-pr-2455.surge.sh

@jgiardino
Copy link
Contributor

jgiardino commented Jul 3, 2019

These updates look good! The only question I have is whether this would be a breaking change (and if so, are there any changes we can make now without waiting for a breaking change release)? I'm not able to test what happens if rememberMeAriaLabel is defined on <LoginForm> (for some reason, the code editor section is read-only).

@jenny-s51
Copy link
Contributor Author

jenny-s51 commented Jul 3, 2019

@jgiardino thank you for your feedback! I'm curious as well -- @tlabaj would this be a breaking change?

@jenny-s51 jenny-s51 self-assigned this Jul 3, 2019
@jenny-s51 jenny-s51 added Breaking change 💥 this change requires a major release and has API changes. Do Not Merge labels Jul 3, 2019
dgutride
dgutride reviewed Jul 5, 2019
View reviewed changes
packages/patternfly-4/react-core/src/components/LoginPage/LoginForm.tsx
label={rememberMeLabel}
checked={isRememberMeChecked}
onChange={onChangeRememberMe}
aria-label={rememberMeAriaLabel || rememberMeLabel}
Copy link
Member

@dgutride dgutride Jul 5, 2019

Choose a reason for hiding this comment

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

This looks good to me - let's add one more comment up above in rememberMeAriaLabel (to the comment section noting this propType is now deprecated and is no longer used in the code. Also, please open an issue with a breaking change label indicating we should delete this propType in a future breaking change release.

Copy link
Contributor Author

@jenny-s51 jenny-s51 Jul 5, 2019

Choose a reason for hiding this comment

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

Just pushed again with this addition to the comment. Should be all set now -- made a new breaking change issue as well.

@jenny-s51 jenny-s51 removed Do Not Merge Breaking change 💥 this change requires a major release and has API changes. labels Jul 5, 2019
@tlabaj
Copy link
Contributor

tlabaj commented Jul 5, 2019

Can you mention new issue here please? Thnaks.

@jenny-s51
Copy link
Contributor Author

jenny-s51 commented Jul 5, 2019

@tlabaj It's #2464 🙂

@jenny-s51 jenny-s51 requested a review from dgutride July 8, 2019 13:06
@tlabaj tlabaj assigned jessiehuff and unassigned jenny-s51 Jul 10, 2019
tlabaj
tlabaj approved these changes Jul 11, 2019
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@tlabaj tlabaj merged commit 3cf57e3 into patternfly:master Jul 11, 2019
@patternfly-build
Copy link
Collaborator

patternfly-build commented Jul 11, 2019

Your changes have been released in:

  • @patternfly/react-core@3.70.1
  • @patternfly/react-docs@4.8.74
  • @patternfly/react-inline-edit-extension@2.9.37
  • demo-app-ts@2.11.1
  • @patternfly/react-integration@2.11.1
  • @patternfly/react-table@2.14.11
  • @patternfly/react-topology@2.6.8
  • @patternfly/react-virtualized-extension@1.1.70

Thanks for your contribution! 🎉

@jenny-s51 jenny-s51 mentioned this pull request Jul 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tlabaj tlabaj tlabaj approved these changes

@dlabaj dlabaj Awaiting requested review from dlabaj

@jgiardino jgiardino Awaiting requested review from jgiardino

+1 more reviewer

@dgutride dgutride dgutride approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jessiehuff jessiehuff

Labels

A11y

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

LoginPage - remove aria-label from checkbox

6 participants

@jenny-s51 @patternfly-build @jgiardino @tlabaj @dgutride @jessiehuff