-
Notifications
You must be signed in to change notification settings - Fork 433
Datepicker maintain focus prop #2391
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
Datepicker maintain focus prop #2391
Conversation
Thanks for opening this pull request! 💯 This is a community-driven project, and we can't do it without your participation. Please check out our contributing guidelines and review the Contributor Checklist if you haven't already, to make sure everything is squared away. TravisCI will take about 10 minutes to run through the same items that are on the Contributor checklist with a pass/fail check below. Please fix any issues that cause TravisCI to fail or ask for clarification--we try, but sometimes the errors can be unclear. |
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Ben Fox <b***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce.com Contributor License Agreement and this Pull Request will be revalidated. |
Thanks for the contribution! Before we can merge this, we need @BenjaminWFox to sign the Salesforce.com Contributor License Agreement. |
Sorry, this appears to have some commits from something I playing with some time ago. I will see if I can remove them from this PR, otherwise I will close this and submit a new PR. ETA: junk commits removed. |
3ccce47
to
a35430f
Compare
Thank you so much for carrying this forward! @BenjaminWFox Apologies for the delay on the feedback. I think this is good pattern. It can be the default without the prop. You can remove the new prop. The primary issue I see is that explicitly opening the menu with the right side icon should still focus on the menu day. It is currently not. The focus is staying on the icon Something like a And please run |
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.
Please see request left in previous comment.
3facc4d
to
c4ddb70
Compare
…ected/interacted with
… focus. Update autogenerated docs
c4ddb70
to
c80a06e
Compare
@interactivellama This is updated per your suggestions, let me know if any further edits are needed, thanks. |
I pulled down and viewed in Storybook and the UX pattern is looking great! Give me a few days to look over the code and we should be good. |
Congrats on merging your first pull request to Design System React! 🎉 |
Thank you @BenjaminWFox for your work! |
Fixes #2285
Additional description
Currently in the datepicker, interaction with the
Input
element moves focus to theCalendar
. This PR adds an optional propcanInputMaintainFocus
to theDatepicker
which, whentrue
, allows clicking & interacting with the input element without losing focus.I'm unsure if solving this by adding an additional prop is acceptable, but there hadn't been any movement on this issue in the last month so I thought I would try. This could also be adapted into the new default behavior pretty easily.
I believe that with
canInputMaintainFocus
set totrue
all of the pattern bullet points are satisfied.I added a story, but have not added a browser-test explicitly for this as I had trouble simulating the focus event. If that would be a requirement please let me know.
CONTRIBUTOR checklist (do not remove)
Please complete for every pull request
npm run lint:fix
has been run and linting passes.components/component-docs.json
CI tests pass (npm test
).REVIEWER checklist (do not remove)
components/component-docs.json
tests.Required only if there are markup / UX changes
last-slds-markup-review
inpackage.json
and push.last-accessibility-review
, topackage.json
and push.npm run local-update
within locally cloned site repo to confirm the site will function correctly at the next release.