Depends on D5110
Introducing a simple ModalLabel component, which will be used in e.g. SubchannelComposer
Figma:
https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1077%3A70464
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- jakub/channel-composer
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Looks good at a high level! Requested two changes to maintain consistency with the rest of the codebase
| web/components/modal-label.react.js | ||
|---|---|---|
| 3 | We use import * as React from 'react'; elsewhere in the codebase | |
| 8 | nice catch making this read only | |
| 11–15 | We use the function keyword for React components elsewhere in the codebase eg function ModalLabel(props: Props): React.Node { return <h1>Blah</h1>; } | |
Comment Actions
Overall I'm not really sure if introducing components that are that simple is beneficial. It is just a simple div with a class, so maybe it would be better to just have div with a class where this component would be used? (not sure, it depends on the rest of the stack).
Usually, when we have a component which renders a single prop, we can consider using children instead - that is more reusable, but not sure if it applies here.
| web/components/modal-label.css | ||
|---|---|---|
| 2 ↗ | (On Diff #16733) | Usually rem should be preferred over px, but in our case we're using px almost everywhere, and the consistency is more important. |
Comment Actions
After discussion in real life, I decided to abandon this diff and include this component directly to ComposeSubchannel