fix(235) - Move open isolated #434
Conversation
| @@ -1,5 +1,6 @@ | |||
| import React from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
| import { MdFullscreen, MdFullscreenExit, MdArrowDropDown, MdArrowDropUp } from 'react-icons/lib/md'; | |||
There was a problem hiding this comment.
I think we should import particular icon modules, otherwise we’ll have the whole library in the bundle. And wouldn’t rely on tree shaking in Webpack 2 ;-)
There was a problem hiding this comment.
I haven't had problems in importing like this but sure , I can divide this in 4 lines 👍
There was a problem hiding this comment.
Have you measured bundle size before and after? ;-|
| cursor: 'pointer', | ||
| '&:hover, &:active': { | ||
| isolate: false, | ||
| '&:hover': { |
There was a problem hiding this comment.
Probably better to disable isolation then to repeat all properties if it doesn’t break anything.
There was a problem hiding this comment.
isolate: false what does this do ?
There was a problem hiding this comment.
It disables jss-isolate. That’s why you have to copy styles from non-hover state when you don’t have isolate: false.
There was a problem hiding this comment.
OHHHHHHHHHHHHHHHHHH
| right: -1, | ||
| margin: 0, | ||
| padding: [[space[0], space[1]]], | ||
| outline: 'none', |
There was a problem hiding this comment.
If you disable default focus styles you need to provide some fallback to make it accessible with a keyboard.
There was a problem hiding this comment.
I will set a focus with the outline , good catch 👍
| hideCode: { | ||
| composes: '$codeToggle', | ||
| backgroundColor: color.codeBackground, | ||
| toolbar: { |
There was a problem hiding this comment.
react-group ? What do you mean ?
There was a problem hiding this comment.
https://sapegin.github.io/react-group/ — we already use it in Styleguidist.
There was a problem hiding this comment.
But why use react group ?
There was a problem hiding this comment.
Because it’s easy and you could avoid this ugly selector with :not ;-)
There was a problem hiding this comment.
You don't like my css mastery ? :p
There was a problem hiding this comment.
Oh , you made this, you selling me your stuff :p
|
Should we use a branch for the new UI? I don’t want it to block other features and don’t want to release many small changes separately. |
|
@sapegin I think we should but maybe after this PR goes because this one actually closes an issue |
|
This PR alone creates even more inconsistency with opening / closing isolation ;-| |
|
@sapegin I can add the same icon at the top , I tought it would be done in phases but I can dp that really quickly 😄 |
|
That’s why I want to use a branch, so we could iterate not in production ;-) |
|
I created a ui branch and it's pointed to there now 👍 |
Codecov Report
@@ Coverage Diff @@
## ui #434 +/- ##
==========================================
+ Coverage 94.28% 94.29% +0.01%
==========================================
Files 82 82
Lines 1138 1140 +2
Branches 255 257 +2
==========================================
+ Hits 1073 1075 +2
Misses 61 61
Partials 4 4
Continue to review full report at Codecov.
|
|
Changes made @sapegin
|
| <section className={classes.toolbar}> | ||
| <div> | ||
| <Group separator=" " className={classes.toolbar}> | ||
| <div className={classes.toolbarItem}> |
There was a problem hiding this comment.
Do we still need a margin here? If space isn’t enough then react-group is not the best choice because now we’re mixing spaces (a real space) with margins.
There was a problem hiding this comment.
I tried adding only the spaces and they were too close together and you could click one by accident
There was a problem hiding this comment.
Then maybe add padding to the button instead of margin to increase clickable area?
There was a problem hiding this comment.
The thing is I think it makes sense semantically ( not idea if that is spelled correctly) to have their own margins , that way when something is added it always has that margin associated
There was a problem hiding this comment.
That’s a tough question ;-) I often keep margins and layout in a separate layer. So do whatever you prefer now and when we extract the button to a separate component we’ll talk again what to do ;-)
There was a problem hiding this comment.
I added a div with a class of toolbarItem arround the component to actually give the margin some abstration from the component :)
|
I think it’s good enough for the very first iteration, merged, thanks! |
|
🎉 🎉 |
Fixes #235