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

Import Dropdown component; add dark variants for dropdown and text fields #862

Merged
merged 27 commits into from
Sep 5, 2019

Conversation

vdepizzol
Copy link
Contributor

@vdepizzol vdepizzol commented Aug 8, 2019

(This PR is based on #853, but created from a local branch instead of a fork, as suggested by @shawnbot)

input-dark and dropdown-menu-dark are two new classes that are added on top of the existing markup to request a dark variation.

image

image

This fixes github/design-systems#669, closes #566.

/cc @primer/ds-core

@vercel
Copy link

vercel bot commented Aug 8, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://primer-css-git-input-dark.primer.now.sh

pages/css/components/dropdown.md Outdated Show resolved Hide resolved
src/dropdowns/README.md Outdated Show resolved Hide resolved
@jonrohan
Copy link
Member

We're punting this from 12.6 and putting in the next release

@shawnbot shawnbot changed the base branch from release-12.6.0 to master August 19, 2019 18:25
Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

Merged in master to test and with the changes from doctocat, the dropdown styles from Primer CSS get overridden by the dropdown styles from styleguide.css.

image

Maybe it's ok to just switch the source order? I'll make a separate PR for that. Edit: 👉 PR #869

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

Changes LGTM.. 👍

Something we might wanna consider: Temporarily adding input-dark and dropdown-menu-dark as a "/hack" instead of officially introduce them to Primer CSS. Because if/when we add support for dark mode, all components would need a dark version and we might not do that by adding *-dark modifier classes to all components?

@shawnbot
Copy link
Contributor

Something we might wanna consider: Temporarily adding input-dark and dropdown-menu-dark as a "/hack" instead of officially introduce them to Primer CSS. Because if/when we add support for dark mode, all components would need a dark version and we might not do that by adding *-dark modifier classes to all components?

Yeah, I think of this change as a hack too. When we're ready to support dark mode more generally, we can deprecate the dark-specific selectors — unless there's some possibility that we'd want to allow folks to "manually" toggle dark mode for individual components? 🤔

@shawnbot
Copy link
Contributor

shawnbot commented Sep 5, 2019

As of 6964be2, this will have the following impacts on CSS bundles:

  • 54 more selectors
  • 1.04 K larger gzipped
  • 4.2 K larger uncompressed

(As seen in the publish logs)

@vercel vercel bot temporarily deployed to staging September 5, 2019 20:17 Inactive
@shawnbot
Copy link
Contributor

shawnbot commented Sep 5, 2019

@vdepizzol the other thing I noticed when poking around on the docs page (which was probably broken before you started working on this): we need to probably need to wrap all of the examples in a <div style="min-height: 160px"> or similar to ensure that they dropdown is visible:

image

after:

image

To fix the north-oriented ones, you could insert an empty <div style="height: 160px"> before the <details> to push it down. See also: #820, #735 /cc @colebemis

@vdepizzol
Copy link
Contributor Author

@shawnbot just fixed all examples in the dropdown page! flex-justify-center and flex-justify-end were useful to align the examples accordingly. ✨

@shawnbot
Copy link
Contributor

shawnbot commented Sep 5, 2019

Awesome! It looks like the .dropdown-w example is a bit wonky:

image

but that's also the case in production, so 🤷‍♀

image

Also, we don't use dropdown-w anywhere. So maybe we delete it? 😬 nope nope nope don't do it

@vdepizzol
Copy link
Contributor Author

vdepizzol commented Sep 5, 2019

@shawnbot yeah, I also noted this. The spacings for dropdown-e and dropdown-w seem quite weird. Don't know if they are used or not 😬😬.

Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

Okay, this looks great; let's ship it! 🚢 🚀 🤘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants