Update css variable tables #1803
Conversation
gdoyle1
left a comment
gdoyle1
left a comment
There was a problem hiding this comment.
@jschuler This looks SO much better. Can we move the chart colors table above the fonts table on the Documentation / CSS variables page?
As we discussed through Slack, I'll make a follow-up issue to continue iterating on the organization of that page
| this.prefix = | ||
| typeof props.prefix === "string" ? [props.prefix] : props.prefix; | ||
| const applicableFiles = Object.entries(tokensModule) | ||
| .filter(([key, val]) => { |
There was a problem hiding this comment.
What about:
const prefixTokens = this.prefix.map(prefix => prefix.replace("pf-", "").replace(/-+/g, "_")
const applicableFiles = Object.entries(tokensModule)
.filter(([key, _val]) => prefixTokens.includes(key)| } | ||
| ]; | ||
|
|
||
| this.columns = this.columns.concat([ |
There was a problem hiding this comment.
I like avoiding destructuring for performance, but using it is a more readable:
this.columns = [
...props.hideSelectorColumn ? [] : [{
title: "Selector",
transforms: [sortable],
cellFormatters: [expandable]
}]
{ title: "Variable", transforms: [sortable] },
{ title: "React Token", transforms: [sortable] },
{ title: "Value", transforms: [sortable] }
]| } | ||
| }; | ||
|
|
||
| this.getFilteredRows = this.getFilteredRows.bind(this); |
There was a problem hiding this comment.
Why do we need to bind? Are class members not bound automagically? :(
There was a problem hiding this comment.
Not needed for the first 2, last one I changed the function to use arrow syntax so also not needed anymore, thanks
| (val.values && searchRE.test(JSON.stringify(val.values))); | ||
| if (passes) { | ||
| const rowKey = `${selector}_${val.property}`; | ||
| let cells = this.props.hideSelectorColumn ? [] : [selector]; |
There was a problem hiding this comment.
Same thing with destructuring
packages/gatsby-theme-patternfly-org/components/cssVariables/cssVariables.js
Outdated
Show resolved
Hide resolved
packages/gatsby-theme-patternfly-org/components/cssVariables/cssVariables.js
Outdated
Show resolved
Hide resolved
|
Thanks for the review @redallen , addressing your comments now |
|
@redallen , I've addressed your comments and I also changed the grid breakpoints for CSS variables and props to grid-lg (as opposed to grid-md the default) so that they break sooner |
redallen
left a comment
redallen
left a comment
There was a problem hiding this comment.
Looking good like it should! CI is failing because it can't download yarn, I don't think anything is wrong with the code. I'd love to get this in for this release.
redallen
dismissed
gdoyle1’s stale review
Gina told me the order was her only concern and that it's fixed!
Closes patternfly/patternfly-react#3521
http://patternfly-org-v4.surge.sh/v4/