feat(CSSTable): Add CSS variable table #54
Conversation
rebeccaalpert
force-pushed
the
css-table
branch
5 times, most recently
from
dfaddc2 to
ced3f2a
Compare
rebeccaalpert
changed the title
rebeccaalpert
force-pushed
the
css-table
branch
3 times, most recently
from
3b3705b to
df1fe65
Compare
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Some initial comments below
| type Value = { | ||
| name: string | ||
| value: string | ||
| values?: string[] | ||
| } | ||
|
|
||
| type FileList = { | ||
| [key: string]: { | ||
| name: string | ||
| value: string | ||
| values?: Value[] | ||
| } | ||
| } |
There was a problem hiding this comment.
For Value, could we instead type the values property as Value[], then in FileList type the key as Value?
There was a problem hiding this comment.
This is an example of what I'm seeing come back on line 65:
I can't really change these much without digging more into the logic from -org. I was just sort of attempting to type based on outputs I was seeing. I'm totally happy to rename these though.
| <List isPlain> | ||
| <ListItem>{property}</ListItem> | ||
| {values.map((entry: string) => ( | ||
| <ListItem key={entry} icon={<LevelUpAltIcon className="rotate-90-deg" />}> | ||
| {entry} | ||
| </ListItem> | ||
| ))} | ||
| </List> |
There was a problem hiding this comment.
This can be a followup, but we should revisit how we're connecting these items. Right now in Org they're rendered as separate list siblings which I don't thin conveys the relationship well. Either having each item be a nested list of the previous item, or maybe a tree view, might help convey that relationship better.
There was a problem hiding this comment.
Tucking this here so it doesn't get forgotten: #57
rebeccaalpert
force-pushed
the
css-table
branch
2 times, most recently
from
265eaa7 to
58d5b7a
Compare
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
This is great 💪
I have a handful of nits but nothing big:
|
Addressed Austin feedback! |
Co-authored-by: Austin Sullivan <[email protected]>
|
🎉 This PR is included in version 1.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
To run:
Can test at http://localhost:${yourPortHere}/get-started/contribute
Questions:
It seems expand all isn't be used - do we need this? I dropped it.
If types look bizarre, let me know. I am basing it on the data I am seeing, but totally possible it's off. Original wasn't typed. I don't really know what these types need to be.
Do we need any other backend here? I got this hooked up to pull in old docs format.
Screenshot