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

[fw-button] fix a11y issues and standardise attributes #218

Merged
merged 6 commits into from
Oct 8, 2021
Merged

[fw-button] fix a11y issues and standardise attributes #218

merged 6 commits into from
Oct 8, 2021

Conversation

kulkarni-kashyap
Copy link
Contributor

@kulkarni-kashyap kulkarni-kashyap commented Oct 4, 2021 •
edited
Loading

  • Adding capability for keyboard navigation and fix a11y issues for better support to screen readers
  • Adding throttling capability to button click to handle rage clicks

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My commits have standard messages as mentioned in Contributing Guidelines

How Has This Been Tested?

Have tested the changes by rendering component in index.html and running on localhost

@kulkarni-kashyap kulkarni-kashyap changed the title fix a11y issues and standardise attributes [fw-button] fix a11y issues and standardise attributes Oct 4, 2021
manoj-krishnan
manoj-krishnan reviewed Oct 5, 2021
View reviewed changes
Copy link
Contributor

@manoj-krishnan manoj-krishnan left a comment

Choose a reason for hiding this comment

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

For the renamed props, we need to think of a deprecation strategy where we'll also retain the old props as aliases for sometime. Can we explore a simple decorator (something like Alias) which would help us decorate deprecated properties and tie them to the updated properties?

packages/crayons-core/src/components/button/button.tsx Outdated Show resolved Hide resolved
packages/crayons-core/src/utils/utils.ts Outdated Show resolved Hide resolved
@kulkarni-kashyap
refactor(fw-button): refactored throttle login based on timestamp ins…
062c9c9 
…tead of setTimeout

Throttle logic is refactored based on timestamp to avoid unexpected delays and css values change
based on standards
@kulkarni-kashyap
Copy link
Contributor Author

For the renamed props, we need to think of a deprecation strategy where we'll also retain the old props as aliases for sometime. Can we explore a simple decorator (something like Alias) which would help us decorate deprecated properties and tie them to the updated properties?

Have reverted renaming of attributes as discussed

manoj-krishnan
manoj-krishnan suggested changes Oct 7, 2021
View reviewed changes
packages/crayons-core/src/utils/index.ts Outdated Show resolved Hide resolved
packages/crayons-core/src/utils/index.ts Outdated Show resolved Hide resolved
packages/crayons-core/src/utils/index.ts Outdated Show resolved Hide resolved
packages/crayons-core/src/components/button/button.scss Show resolved Hide resolved
@kulkarni-kashyap kulkarni-kashyap marked this pull request as ready for review October 7, 2021 05:14
@kulkarni-kashyap
refactor(fw-button): rename variables in function
d1dd1fd 
Rename variables in function to more meaningful names and provide option to override throttle delay
manoj-krishnan
manoj-krishnan approved these changes Oct 7, 2021
View reviewed changes
Copy link
Contributor

@manoj-krishnan manoj-krishnan left a comment

Choose a reason for hiding this comment

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

Please merge after fixing one minor docs comment.

packages/crayons-core/src/components/button/readme.md Outdated Show resolved Hide resolved
@kulkarni-kashyap
docs(fw-button): updating docs to accommodate latest changes
2061818 
Updating docs to accommodate latest changes after reverting attribute name changes
@kulkarni-kashyap kulkarni-kashyap merged commit 96c2a69 into monorepo Oct 8, 2021
@kulkarni-kashyap kulkarni-kashyap deleted the crayons-button branch October 8, 2021 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manoj-krishnan manoj-krishnan manoj-krishnan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kulkarni-kashyap @manoj-krishnan