-
Notifications
You must be signed in to change notification settings - Fork 9
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
chore(docs): rebuild table from all-tokens json #72
Conversation
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.
Is there anything in the README which described how the tokens table was generated in the docs, which should now be updated?
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 not sure of the process for generating the docs - these token files are out of date so they really shouldn't be checked in. And maybe the docs need to be updated? +1 to @nicolethoen's comment that we need to document the process.
My only other comment is that the menu to filter the selection is a little odd with checkbox and also a check on hover. But I'm fine with refining that later.
Thanks @srambach for the feedback - I'll take a look at addressing your comments, not quite sure what I did to get those double checkmarks so thanks for catching that 🤔 |
c522a98
to
950c490
Compare
0cf8042
to
c60c1b8
Compare
791189d
to
82b2515
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.
A couple of comments, but they don't need to hold things up if you want to get this in.
height: 1em; | ||
display: inline-block; | ||
aspect-ratio: 1 / 1; | ||
border-radius: 50%; |
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 can use this here --pf-t--global--border--radius--pill
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! Opening a follow-up issue to swap this value out 👍
@media (min-width: 1200px) { | ||
.tokens-table-outer-wrapper { | ||
width: calc(92vw - var(--pf-v6-c-page__sidebar--Width)); | ||
max-width: calc(92vw - var(--pf-v6-c-page__sidebar--Width)); |
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 curious about the 92vw. Is this a place where we could use .pf-m-limit-width
on the page section instead?
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 calling this out - our docs limits the width of the wrapper element that this content loads into, that value worked well when testing to increase the width of the table (to make it less squished unnecessarily) but yes we should swap it out for something more dynamic. Opening a follow-up issue to review this suggestion 👏
Preview: Preview link
Closes #68
This PR implements new tokens tables which:
README has been updated to explain commands to execute this code, but in summary the data needed for the table is pulled when
yarn build
oryarn build:scss
is run (they trigger the same code), as two new json files containing all the token data displayed in the table are created by Style Dictionary in the same process as the scss files creation.