-
Notifications
You must be signed in to change notification settings - Fork 139
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
docs(release-highlights): Adds additional updates for v6 release. #4317
Conversation
Did we add a note about prefixing the react-tokens with |
@tlabaj just added! Do you think it's clear to mention that following this heading structure, or too muddy?
Is there any specific reasoning we should mention? I looked through some PRs and Slack threads that were related, but couldn't get a great sense of the reason it needed to happen |
We'll have to rename the Battery component to Severity. We are using our new severity icons now |
cc @mcoker should we give anymore context here about prefixing tokens with |
packages/documentation-site/patternfly-docs/content/get-started/release-highlights.md
Outdated
Show resolved
Hide resolved
|
||
- Data list | ||
- Refactored `<DataListCheck>` to use `<Checkbox>` internally. As a result, `id` is now a required prop for ` <DataListCheck>`. This also fixes broken checkboxes in the examples. | ||
- Dropdown |
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.
So we kind of reverted this. We have a prop to set the time out but it is 0
by default. We added some other logic internally to stop the scroll from jumping when focusing the first item in the menu.
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.
ahh okay, is that focusTimeoutDelay
? How's this
- Dropdown
- Added
focusTimeoutDelay
, which specifies the time to wait before setting focus on the first dropdown item whenshouldFocusFirstItemOnOpen
is set.- Fixed issues with invalid and jumpy scrolling when focusing on the first menu item.
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.
Sounds good @edonehoo
packages/documentation-site/patternfly-docs/content/get-started/release-highlights.md
Outdated
Show resolved
Hide resolved
packages/documentation-site/patternfly-docs/content/get-started/release-highlights.md
Outdated
Show resolved
Hide resolved
|
||
#### React tokens | ||
|
||
To address instances where chart tokens and chart variable names were unintentionally identical, we added a `t_` prefix to our React tokens. This makes it easier to differentiate between tokens and variables. |
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.
Maybe say between design tokens and other CSS variables.
or whatever that actually is, but it's in a React tokens section and referring to design token variables.
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.
Yeah this is a little difficult to understand IMO. Here's a horrible take on making it a little more clear? It's a little tricky with the different usages of the word "token"
To address instances where chart design tokens and chart CSS variables created unintentionally identically named React tokens, we added a
t_
prefix to all design tokens in React tokens. This makes it easier to differentiate between design tokens and CSS variables.
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.
yeah, I'm apparently always going to get confused when writing about tokens and design tokens 😭
this is helpful, not horrible! ty both
so
"we added a t_ prefix to all design tokens in React tokens"
the prefix wasn't added to the React tokens themselves? I thought this part was the React token, am I misunderstanding?
Clarification for this ^ will influence my change suggestion, but this is what I have now
To address instances where design tokens and CSS variables for charts unintentionally created identical React tokens, we added a
t_
prefix to all design token references in our React tokens. This makes it easier to differentiate between design tokens and other CSS variables.
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.
You got it! That update lgtm.
"we added a t_ prefix to all design tokens in React tokens"
the prefix wasn't added to the React tokens themselves? I thought this part was the React token, am I misunderstanding?
Yep it's on the react token. When I sad "all design tokens in react tokens" I'm thinking of it like react tokens are a thing themselves, and are made up of design tokens and CSS vars. So with design token in react tokens, I'm just referring to any of the t_
react tokens.
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! Left some comments but I'm no content expert so feel free to ignore if you don't agree with them 👍👍
|
||
We made many updates to our component groups extension, to improve accuracy, usability, and alignment with PatternFly 6. We've moved its website section to our top-level navigation for better visibility, and also to support necessary sub-navigation. We've conceptually grouped the components into functional categories. Additionally, we renamed some of the components to be more accurate. | ||
|
||
[Check out the updated documentation](/component-groups/about-component-groups), which includes: |
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.
Super nit and shouldn't block or anything IMO, but with link text, I ask myself if the text would make sense to a screen reader user if they told the screen reader to read all of the links on the page, which is something someone might do if they were just trying to navigate around. Link text like "Click here" would be read a bunch of times and not indicate where the link goes without reading the context around it. Related, I wondered if "component groups extension" should be linked in the first sentence of the paragraph above.
But we have a lot of links that are lacking that context in the link text, so maybe that's not something we even want to do.
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.
Eh, not a "super nit", this is important! And something I try to keep in mind, but definitely something I miss often when I'm rushing/spreading myself thin. Even then I tend to think of it from a "it should be obvious what this link is" vs the screenreader perspective, so this is a good reminder about that other side. I'll update this one to your suggestion and also do a quick look over 👀
|
||
#### React tokens | ||
|
||
To address instances where chart tokens and chart variable names were unintentionally identical, we added a `t_` prefix to our React tokens. This makes it easier to differentiate between tokens and variables. |
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.
Yeah this is a little difficult to understand IMO. Here's a horrible take on making it a little more clear? It's a little tricky with the different usages of the word "token"
To address instances where chart design tokens and chart CSS variables created unintentionally identically named React tokens, we added a
t_
prefix to all design tokens in React tokens. This makes it easier to differentiate between design tokens and CSS variables.
packages/documentation-site/patternfly-docs/content/get-started/release-highlights.md
Outdated
Show resolved
Hide resolved
packages/documentation-site/patternfly-docs/content/get-started/release-highlights.md
Outdated
Show resolved
Hide resolved
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.
can we add a bullet about feat(TextInputGroup): added validation support
we also added isPlaceholder
prop to the EditableSelectInputCell
of table patternfly/patternfly-react#10850
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.
We should add information about deprecating tile
and using Card
instead.
patternfly/patternfly-react#10821
@tlabaj is this only for deprecated table? |
you are right, it is for deprecated table. So we don't have to mention that. |
@edonehoo |
|
|
packages/documentation-site/patternfly-docs/content/get-started/release-highlights.md
Outdated
Show resolved
Hide resolved
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.
Looking really good; just a couple of URLs that look like they need a slight adjustment.
packages/documentation-site/patternfly-docs/content/get-started/release-highlights.md
Outdated
Show resolved
Hide resolved
packages/documentation-site/patternfly-docs/content/get-started/release-highlights.md
Outdated
Show resolved
Hide resolved
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 for the fixes! LGTM 🎃
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.
BAM! Awesome!
Closes #4289
Will need input on anything to add / remove, please and ty!