Skip to content

refactor(datepicker)!: diy token migration #2185

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

Merged
merged 32 commits into from
Nov 8, 2023

Conversation

jenndiaz
Copy link
Contributor

@jenndiaz jenndiaz commented Sep 25, 2023

Description

Migrates DatePicker to new tokens.
DatePicker does not have a figma or XD files, however it is similar to Combo box which does.

Additionally:

  • addresses an issue with the invalid icons being hidden behind the picker button (see screenshots)
  • ensures WHCM
  • add quiet variant to storybook
  • as per design feedback, border of the picker button has been updated to match the border of text-field

Jira Ticket 579

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Testing Outline

@jawinn

  1. open the docs site for the DatePicker component:
  • with the exception of the invalid icon positioning, styling should match production
  • Invalid icon should be positioned to the left of the picker button and match styling of invalid icon on ComboBox
  1. open the storybook for the DatePicker component
  • default and quiet variants are displayed
  • controls work as expected

Regression testing

Validate:

  1. A legacy documentation page (such as accordion), including:
  • The page renders correctly
  • The page is accessible
  • The page is responsive
  1. A migrated documentation page (such as action group), including:
  • The page renders correctly
  • The page is accessible
  • The page is responsive

Screenshots

Before

Screenshot 2023-09-28 at 1 19 54 PM
Screenshot 2023-09-28 at 1 20 02 PM

After

Screenshot 2023-10-07 at 8 39 55 AM
Screenshot 2023-10-07 at 8 40 04 AM

To-do list

  • I have read the contribution guidelines.

  • I have updated relevant storybook stories and templates.

  • I have tested these changes in Windows High Contrast mode.

  • If my change impacts other components, I have tested to make sure they don't break.

  • If my change impacts documentation, I have updated the documentation accordingly.

  • ✨ This pull request is ready to merge. ✨

@jenndiaz jenndiaz force-pushed the jenndiaz/css-579-date-picker-diy branch from c345711 to 5571c71 Compare September 26, 2023 14:58
@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2023

🚀 Deployed on https://pr-2185--spectrum-css.netlify.app

@github-actions github-actions bot temporarily deployed to pull request September 26, 2023 15:49 Inactive
@jenndiaz jenndiaz force-pushed the jenndiaz/css-579-date-picker-diy branch from 80948c4 to 044ba38 Compare September 27, 2023 16:41
@github-actions github-actions bot temporarily deployed to pull request September 27, 2023 16:47 Inactive
@jenndiaz jenndiaz changed the title feat(datepicker)!: migrate to spectrum tokens feat(datepicker): diy token migration Sep 27, 2023
@github-actions github-actions bot temporarily deployed to pull request September 27, 2023 18:30 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 28, 2023 17:48 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 28, 2023 18:38 Inactive
@jenndiaz jenndiaz changed the title feat(datepicker): diy token migration refactor(datepicker)!: diy token migration Sep 28, 2023
@jenndiaz jenndiaz marked this pull request as ready for review September 28, 2023 19:33
@jenndiaz jenndiaz requested review from jawinn, pfulton and mdt2 September 28, 2023 19:33
@github-actions github-actions bot temporarily deployed to pull request September 28, 2023 20:39 Inactive
Copy link
Collaborator

@jawinn jawinn left a comment

Choose a reason for hiding this comment

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

This looks great. The only thing I see that needs adjustment is related to the spacing around the invalid icon. The quiet in particular has too much right padding, which causes the values to turn to ellipsis sooner than they should:
Screenshot 2023-10-02 at 12 00 45 PM
The regular one ("Range", invalid) also has 2px more padding than combobox has between the text and the invalid icon, which when fixed would mean a value like "10/30/2022" would no longer turn to ellipsis there. The placeholder format still would though, which leads me to...

Possible design change:
I'm wondering if it would be a good idea to increase the overall min-width of the range inputs, so that the placeholders and values do not turn to ellipsis when there is an invalid icon. This may be something to validate with the design team, as it'd be a change from what it is now. Otherwise the user experience of visually comparing what might be wrong or entering a value is hindered by not being able to see the entire value or expected format.

High contrast, quiet:
I saw this as well; the invalid icon is a little too close to the button border:
Screenshot 2023-10-02 at 12 49 40 PM

@github-actions github-actions bot temporarily deployed to pull request October 3, 2023 16:54 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 5, 2023 14:41 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 5, 2023 17:06 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 5, 2023 19:38 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 7, 2023 14:36 Inactive
@jenndiaz
Copy link
Contributor Author

jenndiaz commented Oct 7, 2023

This looks great. The only thing I see that needs adjustment is related to the spacing around the invalid icon. The quiet in particular has too much right padding, which causes the values to turn to ellipsis sooner than they should: Screenshot 2023-10-02 at 12 00 45 PM The regular one ("Range", invalid) also has 2px more padding than combobox has between the text and the invalid icon, which when fixed would mean a value like "10/30/2022" would no longer turn to ellipsis there. The placeholder format still would though, which leads me to...

Possible design change: I'm wondering if it would be a good idea to increase the overall min-width of the range inputs, so that the placeholders and values do not turn to ellipsis when there is an invalid icon. This may be something to validate with the design team, as it'd be a change from what it is now. Otherwise the user experience of visually comparing what might be wrong or entering a value is hindered by not being able to see the entire value or expected format.

High contrast, quiet: I saw this as well; the invalid icon is a little too close to the button border: Screenshot 2023-10-02 at 12 49 40 PM

I ensured the values for the right padding on the inputs matches what we are using for Combobox. This address the high contrast right padding and lessened the impact of the right padding on trimming the placeholder text. I ran it by design and they suggested this is okay since those using the component can adjust the width of the inputs if desired. They also pointed out an issue with the button border not matching the input border which I addressed as well.

Slack conversation with design

@jenndiaz jenndiaz requested a review from jawinn October 7, 2023 14:37
@github-actions github-actions bot temporarily deployed to pull request October 9, 2023 21:51 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 9, 2023 22:27 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 9, 2023 22:46 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 10, 2023 15:30 Inactive
mdt2 pushed a commit that referenced this pull request Oct 11, 2023
Batch adding component-specific custom tokens from PRs #2164, #2168, #2185, #2175, #2188, and #1971
mdt2 pushed a commit that referenced this pull request Oct 11, 2023
Batch adding component-specific custom tokens from PRs #2164, #2168, #2185, #2175, #2188, and #1971
@jenndiaz jenndiaz force-pushed the jenndiaz/css-579-date-picker-diy branch from dd7d716 to ddb638f Compare November 8, 2023 19:34
@pfulton pfulton added the run_vrt For use on PRs looking to kick off VRT label Nov 8, 2023
@github-actions github-actions bot temporarily deployed to pull request November 8, 2023 19:42 Inactive
@github-actions github-actions bot removed the run_vrt For use on PRs looking to kick off VRT label Nov 8, 2023
@pfulton pfulton merged commit 7de0da2 into main Nov 8, 2023
@pfulton pfulton deleted the jenndiaz/css-579-date-picker-diy branch November 8, 2023 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants