-
Notifications
You must be signed in to change notification settings - Fork 143
docs(spacers): Documents recently added layout spacers. #4730
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
base: main
Are you sure you want to change the base?
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.
LGTM! Left a comment about layouts since "layout" as a concept are different from "layouts" as a product offering (our grid, gallery, flex, stack, etc layouts), in case that wasn't clear.
- **Control spacers:** Used to set horizontal and vertical padding within controls, like menu toggles and text inputs. | ||
- Control spacer tokens begin with `--pf-t--global--spacer--control--` | ||
- Control spacer tokens begin with `--pf-t--global--spacer--control` | ||
- **Layout spacers:** Used to define spacing with [PatternFly layouts](/layouts/about-layouts). |
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.
For the layouts we offer, we we pretty much only use gap and gutter spacers. Layouts really only introduce space between elements, not within them (insets are for space inside of something). So if "layout spacers" refers to the layouts listed on https://www.patternfly.org/layouts/about-layouts, then we would remove "inset spacers" from the list and add "gap spacers" if we wanted to be technically correct.
However, if we aren't referring to those specific layouts, and we're using "layout spacers" as a general concept, I think what you have works, except I might still add "gap spacers." IMO I like referring to "layout spacers" this way as a general concept, and not specific to the list of layouts we offer.
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.
👍
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.
nice!
Works towards #4622
But remaining pr is needed to update tokens table description to close the og issue