-
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
Release 9.5.0 #353
Release 9.5.0 #353
Conversation
Style buttons as selected on <details open>
- primer-alerts@1.5.0 - primer-avatars@1.4.0 - primer-base@1.5.0 - primer-blankslate@1.4.0 - primer-box@2.5.0 - primer-breadcrumb@1.4.0 - primer-buttons@2.4.0 - primer-cards@0.5.0 - primer-core@6.4.0 - primer-css@9.5.0 - primer-forms@1.4.0 - primer-labels@1.5.0 - primer-layout@1.4.0 - primer-markdown@3.7.0 - primer-marketing-type@1.4.0 - primer-marketing-utilities@1.4.0 - primer-marketing@5.4.0 - primer-navigation@1.4.0 - primer-page-headers@1.4.0 - primer-page-sections@1.4.0 - primer-product@5.4.0 - primer-support@4.4.0 - primer-table-object@1.4.0 - primer-tables@1.4.0 - primer-tooltips@1.4.0 - primer-truncate@1.4.0 - primer-utilities@4.8.0
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.
Left a few comments 💬
CHANGELOG.md
Outdated
# 9.5.0 | ||
|
||
### Added | ||
- Thanks to @muan, it's now possible to style `<summary>` elements as buttons and have them appear in the active/selected state when the enclosing [`<details>` element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details) is open. #346 |
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 feels inconsistent with our format of "added", "changed" etc. To thank people I'd rather have a "thanks to contributions from..." section for each release. For example @JasonEtco
contributed to the last release and we didn't thank him 😢
modules/primer-buttons/README.md
Outdated
@@ -245,6 +245,21 @@ Use `.hidden-text-expander` to indicate and toggle hidden text. | |||
|
|||
You can also make the expander appear inline by adding `.inline`. | |||
|
|||
|
|||
#### `<details>`/`<summary>` button styles |
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.
I don't think we should use code snippets in the title, that will be a bit odd when rendered in the toc
and will look odd in the title.
modules/primer-buttons/README.md
Outdated
|
||
You can apply `.btn` classes to `<summary>` elements, and they the | ||
selected/active styles will be applied when the parent `<details>` is open. | ||
|
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.
Thinking about it, I'm not sure these docs fit here, I think they'd fit better with the details component when we add that module to Primer.
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.
I'm on the fence about this. My original sense was that people might want to know where they can use button styles, and that this doesn't exactly overlap with the Details component because it doesn't use any of those classes. How would you feel about leaving this here for now so we can get the release out, then addressing its placement in a future release?
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.
Left some suggestions for changes to docs as wasn't feeling quite right. I'm cool with shipping the docs here, I think we should also add this example to the details element docs when we work on those too though.
modules/primer-buttons/README.md
Outdated
@@ -245,6 +245,21 @@ Use `.hidden-text-expander` to indicate and toggle hidden text. | |||
|
|||
You can also make the expander appear inline by adding `.inline`. | |||
|
|||
|
|||
#### Details element summary buttons |
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.
Suggest changing to Using button styles with the details summary element
so that it's more "button focussed" 😏
modules/primer-buttons/README.md
Outdated
#### Details element summary buttons | ||
|
||
You can apply `.btn` classes to `<summary>` elements, and they the | ||
selected/active styles will be applied when the parent `<details>` is open. |
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.
Looks like there are some typos here and could do with some tweaks. Suggest:
You can apply the
btn
class to the<summary>
element so that it gains the appearance of a button, and selected/active styles when the parent<details>
is open.
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.
Thanks. I meant that you can actually use btn
and any of the btn-*
classes, but that wasn't clear.
@broccolini thanks for the feedback! Can you look at the changes in c996a22 and approve if they're good? |
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.
👍 LGTM!
Changes
<summary>
elements as buttons and have them appear in the active/selected state when the enclosing<details>
element is open. Style buttons as selected on <details open> #346TODO
primer-support
(and all dependents) by a minor version<details>
button additionCHANGELOG.md
with v9.4.0 and v9.5.0 changes/cc @primer/ds-core