-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
React component conversion(class to functional) - 2 #3209
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
Conversation
Release EnvironmentsThis Environment is provided by Release, learn more! 🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-73ab94923a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this!
I loaded this PR locally, and everything seems to generally work well for me. The overall code changes look okay to me as well, but will defer to @khanniie's review!
One other thing to note (more so in upcoming PRs)—it would be great to add more specific details or context surrounding the changes for each file in the PR description. This would be helpful to get a better understanding of these changes and for documentation purposes!
@@ -10,8 +10,21 @@ const Item = ({ isAdded, onSelect, name, url }) => { | |||
const buttonLabel = isAdded | |||
? t('QuickAddList.ButtonRemoveARIA') | |||
: t('QuickAddList.ButtonAddToCollectionARIA'); | |||
|
|||
const handleKeyDown = (event) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think buttons should do this natively? might not be necessary~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nvm i see it's a div not a button -- i think in that case then we should just let the button within it be focusable / interactive and let the div be a container? meaning we remove the role & onClick/onKeydown handlers and tabindex. let me know if there's something im missing though!
LGTM overall! : ) |
@raclim I'll keep this in mind for the next PRs!! |
Fixes #2709
Changes:
searchBar
andquickAddList
I have verified that this pull request:
npm run lint
)npm run test
)develop
branch.Fixes #123