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

fix(DatePicker): remove decorative svg as a click target #10283

Merged

Conversation

dakahn
Copy link
Contributor

@dakahn dakahn commented Dec 14, 2021

Closes #8865
Closes #9815

The small decorative calendars were click targets. This was causing weird issue with the second calendar opening and setting the first input. Seems to me they should just not receive pointer events at all which seems to address the problem.

Testing / Reviewing

  • click the first input and select a date
  • observe the first input populate with correct date
  • click the second input and select a date
  • observe the second input populate with correct date

🏄🏾

@dakahn dakahn requested review from a team as code owners December 14, 2021 19:07
@netlify
Copy link

netlify bot commented Dec 14, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 1fc3b11

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61bcf300b83d480009a50663

😎 Browse the preview: https://deploy-preview-10283--carbon-react-next.netlify.app

@dakahn dakahn requested review from tay1orjones and removed request for aledavila December 14, 2021 19:07
@netlify
Copy link

netlify bot commented Dec 14, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 1fc3b11

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61bcf300f99fa80008f19cc4

😎 Browse the preview: https://deploy-preview-10283--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Dec 14, 2021

✔️ Deploy Preview for carbon-components-react ready!

🔨 Explore the source changes: 02d8cfc

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61b8eb6bd58a13000792b77d

😎 Browse the preview: https://deploy-preview-10283--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Dec 14, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 1fc3b11

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61bcf3007478530007fe9b43

😎 Browse the preview: https://deploy-preview-10283--carbon-components-react.netlify.app

@tay1orjones
Copy link
Member

tay1orjones commented Dec 14, 2021

@dakahn I just made two updates to this:

  1. Within DatePicker, remove usages of the openCalendar prop that has been removed
  2. Mirror the style changes you made in packages/styles to packages/components

The storybook now shows the intended functionality; clicking the calendar icon is technically clicking/focusing the input, which opens the calendar. Tests pass.

clicking.icon.focuses.input.opens.calendar.mov

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

A couple considerations for avoiding breaking changes.

@tay1orjones
Copy link
Member

Tests were failing because the DatePicker was still using the deprecated openCalendar prop, triggering an unexpected console.warn. I pushed an update to remove it from DatePicker. openCalendar isn't actually called anymore anyway now that the icon has pointer-events: none.

@joshblack
Copy link
Contributor

One usability concern, when I click the input I now get autocomplete showing up on top of the calendar:

image

To dismiss it, I have to click on the calendar. What is expected for these situations? 👀

@tay1orjones
Copy link
Member

@joshblack could you share more on how to get the autocomplete list to pop? I can't repro in chrome, firefox, or safari.

I'm not sure how/why these changes would impact the browser's autocomplete. We could place autocomplete="off" on the DatePickerInput in storybook.

@joshblack
Copy link
Contributor

@tay1orjones I think it's because before we were never interacting with the input, whereas now we are in order to trigger the appearance of the date picker.

As to how to get the autocomplete, great question lol. I have no idea how it's picking up dates in this situation

Copy link
Contributor

@abbeyhrt abbeyhrt left a comment

Choose a reason for hiding this comment

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

Looks good to me! I also wasn't able to recreate the autocomplete @joshblack was experiencing either but auto complete can be fickle. Reminds me of this old issue, solving it isn't super clear cut and I don't personally think it has to prevent this PR from being merged in

@kodiakhq kodiakhq bot merged commit 7be55cd into carbon-design-system:main Dec 17, 2021
@abbeyhrt abbeyhrt mentioned this pull request Jan 14, 2022
22 tasks
kennylam added a commit to kennylam/carbon that referenced this pull request Jul 30, 2024
* chore(npm): update version to beta

* chore(workflow): add gh-pages publishing workflow

* Update .github/workflows/deploy-cwc-v2-beta.yml

Co-authored-by: andrew <emyarod@users.noreply.github.com>

* Update .github/workflows/deploy-cwc-v2-beta.yml

Co-authored-by: andrew <emyarod@users.noreply.github.com>

---------

Co-authored-by: andrew <emyarod@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants