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

[hidden]{display:none !important;} #467

Closed
shawnbot opened this issue Apr 4, 2018 · 4 comments
Closed

[hidden]{display:none !important;} #467

shawnbot opened this issue Apr 4, 2018 · 4 comments

Comments

@shawnbot
Copy link
Contributor

shawnbot commented Apr 4, 2018

We should add this to primer-base to better support custom elements, and potentially increase the selector specificity to override our d-* utility classes. This will encourage custom elements and JS behaviors to toggle visibility with the hidden attribute rather than toggling the d-none utility class.

As more custom elements are extracted from github.com and designed to function separately from Primer, and as we consider other environments (such as React), we might even discuss deprecating d-none entirely. 😬

/cc @muan

@shawnbot
Copy link
Contributor Author

Another way we could tackle this would be to change our d-* utility classes to, for instance:

.d-flex:not([hidden]) { display: flex; }

The problem I foresee is that if we put [hidden] { display: none !important; } in primer-base, it won't work as-is because the selectors have the same specificity. So if we're going to do this, the [hidden] rule will have to come after .d-* in the resulting CSS.

@shawnbot
Copy link
Contributor Author

This actually begs the question: should we deprecate d-none in favor of hidden? That might be the best way to encourage good practice, and make it easier to move forward with a refactor like https://github.com/github/github/pull/88496.

@shawnbot shawnbot changed the title [hidden] { display: none !important; } [hidden]{display:none !important;} Jul 30, 2018
@shawnbot
Copy link
Contributor Author

Done in #557! We should update the docs to reflect this, maybe with a clear set of rules like:

  1. Use the hidden attribute and JS property if you're going to programmatically show and hide content.
  2. Use d-none and/or its responsive variants (d-sm-block, d-lg-none) to conditionally show content at different screen sizes.

shawnbot added a commit that referenced this issue Aug 23, 2018
@shawnbot
Copy link
Contributor Author

Closing because this is done. Thanks, @muan!

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

No branches or pull requests

1 participant