-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Drop “display: none” from details element contents styling #2624
Drop “display: none” from details element contents styling #2624
Conversation
🦋 Changeset detectedLatest commit: 887edd8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
62952f8
to
887edd8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was likely around for IE11 which we've long since dropped support for. Originally added in #371 in 2017.
As far as I can tell all browsers we see traffic GitHub support <details>
and therefore don't need this. So 👍 from me.
Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days. |
Friendly ping to reviewers |
Thanks for the ping @sideshowbarker. I’ll look at integrating this tomorrow and if it works out nicely we can look to ship it this week. |
What are you trying to accomplish?
Fixes #2592
What approach did you choose and why?
Removed
display: none
fromdetails
contents stylingWhat should reviewers focus on?
See #2592 (comment) for repro/test steps
Can these changes ship as is?