Terynk/theme designer code refactor #1
Conversation
There was a problem hiding this comment.
Great steps!
In another, dedicated PR we should recreate a New ComponentStickerSheet component to replace Demo.tsx. I think you inherited a lot of unnecessary nesting and other code that complicates what should be a really simple component. A clean sheet rewrite will be MUCH faster to do.
Let's chat offline if you wanna talk strategies on layout.
Not signing this one because we don't want it going in with the extra unrelated changes.
| <Tab value="tab1">Home</Tab> | ||
| <Tab value="tab2">Pages</Tab> | ||
| <Tab value="tab3">Documents</Tab> | ||
| {tabValues.map((tab, index) => ( |
There was a problem hiding this comment.
Putting things in a loop is often the best choice. However, I think this is an instance where keeping it written out is probably the better path. Reason being it's less code and way more readable. Quick and easy reading is often more important in demo code than elegance.
| position: 'absolute', | ||
| top: '0px', | ||
| right: '0px', | ||
| width: '400px', |
There was a problem hiding this comment.
I know this is not your code, but in the interest of wax on, wax off....
I like to define file level semantic constants anytime a value gets to be significantly larger than the general tokens. And I often define them for things smaller than that as well.
This helps future readers understand where this "Big random number" came from, but more importantly that the decision was carefully considered and coordinated. The name leaves a breadcrumb trail of how to get there.
There was a problem hiding this comment.
again, NYC, but same idea with z-indexes.
| </Button> | ||
| </div> | ||
| <div className={styles.panelContainer}> | ||
| <ExportPanelHeader onClose={onCloseExportPanel} /> |
There was a problem hiding this comment.
I see you also split things out into separate components here. While I think the intention is noble, it may be making things harder to reason about for a component this size.
| </div> | ||
| <div className={styles.panelContainer}> | ||
| <ExportPanelHeader onClose={onCloseExportPanel} /> | ||
| <br /> |
There was a problem hiding this comment.
I know this is not your code, but we should remove random line breaks.
They're often improperly used by people unfamiliar with CSS layout techniques to give margin or spacing.
They really should only be used in long form, paragraph text content.
| </AccordionPanel> | ||
| </AccordionItem> | ||
| {/* | ||
| The accessibility check is not adequate for the theme designer. |
There was a problem hiding this comment.
Hey these are my comments :D
(yes, good to remove all together)
| }); | ||
|
|
||
| const ExportPanelHeader = ({ onClose }: { onClose: () => void }) => { | ||
| const styles = useStyles(); |
There was a problem hiding this comment.
nit, nyc: I like to add the component name to the style function. This helps us share them clearly later on.
So this one would be useExportPanelHeaderStyles.
This is a habbit from writing shared library code that often ends up benefiting product vertical code.
| 1) Do we have all the correct components displayed and in the correct order? | ||
| 2.a) Do we have all the states of each component displayed? i.e. missing deactive states? | ||
| 2.b) Do was have all the variants of each component displayed? i.e. different sizes, colors, layouts etc? | ||
| 3) Note that the spinner was removed since it was causing confusing with the loading state of the page |
There was a problem hiding this comment.
You can probably remove these comments as well.
New Behavior
Updated Demo.tsx
Updated ExportPanel.tsx
Updated Form.tsx
Other
feature/theme-designer-refactorbranch to improve organization