PCD-1309 Fix/css refactor 2 #326
Conversation
qingyashu
force-pushed
the
fix/css-refactor-2
branch
from
4a0fc27 to
2708f41
Compare
qingyashu
force-pushed
the
fix/css-refactor-2
branch
from
fdb16a7 to
8bfcab7
Compare
| </button> | ||
| </ToggleButton> | ||
| <div className="data-model-graph"> | ||
| <button |
There was a problem hiding this comment.
do we need to add the styles from ToggleButton?
There was a problem hiding this comment.
ToggleButton only has 2 css attribute top and left, They don't have any effect for the button in this page so I directly remove them
| .explorer-component { | ||
| display: flex; | ||
| padding: 40px 0; | ||
| background-color: #ecebeb; |
There was a problem hiding this comment.
where did this color come from?
| <LoginFrame> | ||
| <LeftBox style={{ height: `${this.state.height}px` }}> | ||
| <div className="login-page"> | ||
| <div className="login-page__side-box"> |
There was a problem hiding this comment.
do we need the height that was being passed to LeftBox?
There was a problem hiding this comment.
height is not used, so removed.
| @@ -61,7 +52,6 @@ export function intersection(aList, bList) { | |||
| * @param history from react-router | |||
| * @param match from react-router.match | |||
| * @param public default false - set true to disable auth-guard | |||
| * @param background passed through to <Box background> wrapper for page.jsx-level background | |||
There was a problem hiding this comment.
do we need this param for anything?
There was a problem hiding this comment.
I did some global searching, and found that background-color is only different for ExplorerComponent and was passed via background variable. So to make code cleaner I set background-color inside ExplorerComponent.less, and remove this background parameter from ProtectedContent component.
There was a problem hiding this comment.
awesome, makes sense to me
src/components/layout/NavBar.jsx
Outdated
| <NavLogo> | ||
| <div className="nav-bar"> | ||
| <header className="nav-bar__header"> | ||
| <nav className="nav-bar__nav nav-bar__nav--left"> |
There was a problem hiding this comment.
it doesn't look like nav-bar__nav is a class
There was a problem hiding this comment.
😆 Ah I am really bad at naming! how about nav-bar__content, nav-bar__content--left...
There was a problem hiding this comment.
the name is fine, I just didn't see the class anywhere in the .less file
src/components/layout/NavBar.less
Outdated
|
|
||
| .nav-bar__nav--right .nav-bar__link { | ||
| border-left: 1px solid #d1d1d1; | ||
| &:last-child { |
There was a problem hiding this comment.
Thanks! I will unnest .nav-bar__nav--right .nav-bar__link.
But I think it is reasonable to nest pseudo class names like :hover or :active because they only effect element itself and won't cause any side effects. Can we keep them?
There was a problem hiding this comment.
yeah I get that, but it doesn't don't adhere to the BEM rules, and also doesn't give us the preprocessor speed.
There was a problem hiding this comment.
I think you are right about preprocessor speed, That's a good reason!
Also if we want to have csslint in future then we should clean all & first (will be in next pr)
But I think BEM rule still allow reasonable nesting cases like this one: http://getbem.com/faq/#block-modifier-affects-elements
src/components/layout/NavButton.less
Outdated
| display: inline-block; | ||
| text-align: center; | ||
| opacity: 0.8; | ||
| &:hover { |
src/components/layout/TopBar.less
Outdated
| @@ -0,0 +1,32 @@ | |||
| .top-bar { | |||
| width: 100%; | |||
| background-color: #3283c8; | |||
src/components/layout/TopBar.less
Outdated
|
|
||
| .top-bar__link { | ||
| border-right: 2px solid #6896c6; | ||
| &:last-child { |
| { GA.init(gaTracking, dev, gaDebug) && <RouteTracker /> } | ||
| <ReduxTopBar /> | ||
| <ReduxNavBar /> | ||
| <Box background={background} style={{ width: '100%', margin: 'auto' }}> |
There was a problem hiding this comment.
what happens with this background prop? should we get rid of the variable?
There was a problem hiding this comment.
See comments in src/Login/ProtectedContent.jsx, I think this background variable was not a good design, think we better get rid of it.
|
Thanks for reminding me!! color fixed~ |
No description provided.