[docs] Fix Tabs examples typography & standardise code #9366
Conversation
mbrookes
changed the title
mbrookes
added
PR: needs revision
oliviertassinari
added
component: tabs
| <TabContainer dir={theme.direction}>Item Two</TabContainer> | ||
| <TabContainer dir={theme.direction}>Item Three</TabContainer> | ||
| </SwipeableViews> | ||
| <Typography type="body1"> |
There was a problem hiding this comment.
I would rather put it in the TabContainer component.
There was a problem hiding this comment.
I considered that, but thought it would result in muliple additional components being mounted all applying the same style. I'll go with your lead on it.
There was a problem hiding this comment.
Not much more effort if you replace the div with a <Typography component="div">.
| {value === 4 && <TabContainer>Item Five</TabContainer>} | ||
| {value === 5 && <TabContainer>Item Six</TabContainer>} | ||
| {value === 6 && <TabContainer>Item Seven</TabContainer>} | ||
| <Typography type="body1"> |
There was a problem hiding this comment.
I would rather put it in the TabContainer component.
|
Out of curiosity, is there a reason we can't set this at the top level as the default Typography? |
@mbrookes First reason I can think of is the default element used: |
|
What is more likely in a user app, setting a default font and size (with CSS / JSS global etc, or using I don't want to add |
|
We allow both patterns. I avoid global CSS as much as possible in the projects I'm working on. It's much more common with Bootstrap. Still, the ease of use makes consecutive changes very painful. |
| <Tab value="one" label="New Arrivals in the Longest Text of Nonfiction" /> | ||
| <Tab value="two" label="Item Two" /> | ||
| <Tab value="three" label="Item Three" /> | ||
| <Tab label="New Arrivals in the Longest Text of Nonfiction" /> |
There was a problem hiding this comment.
Did you explicitly remove the value? Or it's a mistake?
There was a problem hiding this comment.
I removed it for consistency with the other examples.
There was a problem hiding this comment.
OK, we will add a visual regression tests if we notice a regression in the future.
|
@oliviertassinari I hope this is what you meant regarding using |
| @@ -42,12 +43,14 @@ class BasicTabs extends React.Component { | |||
| <Tabs value={value} onChange={this.handleChange}> | |||
| <Tab label="Item One" /> | |||
| <Tab label="Item Two" /> | |||
| <Tab label="Item Three" href="#basic-tabs" /> | |||
There was a problem hiding this comment.
This was added on purpuse to show the feature.
|
Here is my take, let me know what you think. 519978b so we can revert in case. |
|
@oliviertassinari Sure - I had tried it that way originally. Both are valid, but that mounts extra components in the DOM, so I went with the slightly broader wrapper, then revised with your |
|
Oh, no I didn't look properly. That is better. 👍 |
|
@mbrookes Feel free to continue the effort if you find other issues :). |
Before:
After:
