Dialog+tabs #773
Dialog+tabs #773
Conversation
✅ Deploy Preview for bldrs-share ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/Components/Dialog.jsx
Outdated
| @@ -40,7 +40,7 @@ export default function Dialog({ | |||
| onClose={close} | |||
| PaperProps={{variant: 'control'}} | |||
| > | |||
| <CloseButton onClick={close}/> | |||
| <CloseButton onClick={close} className='icon-share'/> | |||
There was a problem hiding this comment.
Mmm.. this class should only go on icons, like . That's how I have it everywhere in the refactor I did and we should keep that as an invariant.
In general as well, if we have a class name starting with a component name, like "icon", it should only go directly on a component of that type. Just to keep sanity
|
|
||
| const tabList = ['Explore', 'Open', 'Save'] | ||
| const contentList = [ | ||
| <Box sx={{textAlign: 'left'}} key={1}> |
There was a problem hiding this comment.
These left aligns feel more semantic than style, as in "block text" means something different, like it's prose to read.
'h5' is also semantic, meaning a 5th-level header.
I think what we want here is to replace Box+Typography with just a for Paragraph.
Isn't this whole section
| headerText={'Here\'s the thing!'} | ||
| isDialogDisplayed={true} | ||
| // eslint-disable-next-line no-empty-function | ||
| setIsDialogDisplayed={() => {}} |
There was a problem hiding this comment.
For fixtures, tests, etc. where we have an empty function I've been putting e.g. debug().log('setIsDialogDisplayed')
src/Components/DialogTabs.jsx
Outdated
| isDialogDisplayed, | ||
| setIsDialogDisplayed, | ||
| actionTitle, | ||
| actionCb = [() => alert('loading')], |
There was a problem hiding this comment.
This I think should be required and asserted instead of a default. We don't want to be refactoring and forget to pass a handler but then get an alert in prod.
There was a problem hiding this comment.
If the default prop is not passed, there is an error.
There was a problem hiding this comment.
Since we are using the state as the array position, if the default prop of the array is not passed there is a moment on load when the undefined error is thrown.
There was a problem hiding this comment.
per convo, rename to actionCbArr
src/Components/DialogTabs.jsx
Outdated
| import {assertDefined} from '../utils/assert' | ||
|
|
||
|
|
||
| /** |
There was a problem hiding this comment.
I was having a look at refactoring this and one thing I think helped is to make most props optional. Only things that are really required are:
- headerText
- tabList
- contentList
- actionCb (actionTitle can be 'Ok' by default')
There was a problem hiding this comment.
how do you make props optional, with the assert?
src/Components/DialogTabs.jsx
Outdated
| * @property {React.ReactElement} [actionIcon] Optional icon for the action button | ||
| * @return {React.Component} | ||
| */ | ||
| export default function Dialog({ |
src/Components/DialogTabs.jsx
Outdated
| isDialogDisplayed, | ||
| setIsDialogDisplayed, | ||
| actionTitle, | ||
| actionCb = [() => alert('loading')], |
There was a problem hiding this comment.
Since we are using the state as the array position, if the default prop of the array is not passed there is a moment on load when the undefined error is thrown.
src/Components/DialogTabs.jsx
Outdated
| import {assertDefined} from '../utils/assert' | ||
|
|
||
|
|
||
| /** |
There was a problem hiding this comment.
how do you make props optional, with the assert?
This PR introduces 'MultiTab Dialog' component and 'Tab' component.
Cosmos Fixtures:
MultiTab Dialog:
https://deploy-preview-773--bldrs-share.netlify.app/cosmos/?fixtureId=%7B%22path%22%3A%22src%2FComponents%2FDialogTabs.fixture.jsx%22%7D
Tabs:
https://deploy-preview-773--bldrs-share.netlify.app/cosmos/?fixtureId=%7B%22path%22%3A%22src%2FComponents%2FTabs.fixture.jsx%22%7D
Figma :
https://www.figma.com/file/DqKVaQEukf8vVHnlwiRGhv/Screens?type=design&node-id=2362-16501&mode=design&t=Eh9mFy0P3W79Mk99-0