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

"Dropdown" component - CSS cleanup (02) #202

Merged
merged 11 commits into from
Apr 19, 2022

Conversation

didoo
Copy link
Contributor

@didoo didoo commented Apr 11, 2022

📌 Summary

Some minor cleanup/refactoring of the CSS for the Dropdown component.

🛠️ Detailed description

👀 How to review

👉 Review commit-by-commit

Reviewer's checklist:

  • +1 Percy if applicable
  • Confirm that PR has a changelog update via Changesets if needed

💬 Please consider using conventional comments when reviewing this PR.


…e text

 (the reason is that the parent has position absolute so no intrinsic width)
…classes

(it was not only duplicating the import of CSS classes, but also coming after the components CSS, so resetting margin/padding applied to text elements)
(to avoid having two distinct places where this size is declared)
@didoo didoo requested a review from MelSumner April 11, 2022 11:06
@changeset-bot
Copy link

changeset-bot bot commented Apr 11, 2022

⚠️ No Changeset found

Latest commit: 18a44b8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Apr 11, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

hds-flight-website – ./

🔍 Inspect: https://vercel.com/hashicorp/hds-flight-website/BQDnGaxv2Z1yMey45jQApdaJqJqm
✅ Preview: https://hds-flight-website-git-dropdown-02-css-cleanup-hashicorp.vercel.app

hds-components – ./

🔍 Inspect: https://vercel.com/hashicorp/hds-components/GE8iCbtvLgSa6cMDanpesTU6qqZg
✅ Preview: https://hds-components-git-dropdown-02-css-cleanup-hashicorp.vercel.app

@vercel vercel bot temporarily deployed to Preview – hds-components April 11, 2022 13:52 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website April 11, 2022 13:52 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website April 12, 2022 08:58 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components April 12, 2022 08:58 Inactive
@@ -113,33 +118,32 @@
box-shadow: var(--token-surface-high-box-shadow);
list-style: none;
margin: 0;
max-width: 25rem;
min-width: 12.5rem;
max-width: 400px;
Copy link
Contributor

Choose a reason for hiding this comment

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

[issue] [a11y] these need to be in relative units to support browser zoom.

The most important reason for using responsive and unitless values in our CSS is for supporting our users that rely on zooming. If you read the Web Content Accessibility Guidelines, our users need to be able to zoom the viewport without loss of content or functionality, or restrictions imposed by CSS values or viewport scaling settings.

In particular, there are a some success criteria to be met:

  • WCAG 1.4.4: Users must be able to resize text without assistive technology up to 200 percent, without loss of content or functionality. (Level AA)
  • WCAG 1.4.10: Users must be able to resize text without being forced to scroll both horizontally and vertically to read that content. (Level AA)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MelSumner can you tell me how declaring the size in px or rem would change the outcome in this case (zooming, not changing base font size, which is something we're not able to support yet)?

This is the content at 200% with px

px1

px2

This is the content at 200% with rem

rem1

rem2

They look exactly the same to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per @Dhaulagiri comment I've opened a distinct issue to discuss this, so we are not blocked in the merging of the other PRs.
#228

Copy link
Contributor

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

two small issues, everything else seems correct, although I'm not sure why we're using helper classes? I thought the helper classes were for our users, not us.

Co-authored-by: Melanie Sumner <melanie@hashicorp.com>
@vercel
Copy link

vercel bot commented Apr 19, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
hds-components ✅ Ready (Inspect) Visit Preview Apr 19, 2022 at 10:25AM (UTC)
hds-flight-website ✅ Ready (Inspect) Visit Preview Apr 19, 2022 at 10:25AM (UTC)

@vercel vercel bot temporarily deployed to Preview – hds-flight-website April 19, 2022 10:25 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components April 19, 2022 10:26 Inactive
@didoo didoo requested a review from MelSumner April 19, 2022 14:38
Copy link
Contributor

@Dhaulagiri Dhaulagiri left a comment

Choose a reason for hiding this comment

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

I'm going to approve this to unblock the pipeline of PRs that are open, but let's discuss the pixel/rem question and document our decision so we can refer to that in the future.

Base automatically changed from dropdown/01-add-chevron-animation to dropdown-finalize-implementation April 19, 2022 14:43
@didoo didoo merged commit 688cbe9 into dropdown-finalize-implementation Apr 19, 2022
@didoo didoo deleted the dropdown/02-CSS-cleanup branch April 19, 2022 14:45
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.

3 participants