Migrate to plain CSS (remove CSS modules) #659
Conversation
src/lib/styleHelper.js
Outdated
| @@ -0,0 +1 @@ | |||
| export const prefixer = prefix => className => `ncms-${ prefix }-${ className }`; | |||
There was a problem hiding this comment.
We should document the prefixer.
Is there a runtime impact of this implementation? Is my browser going to be building these strings every time it renders a component? If we're writing the style name in the css file, why not use a static name in the templates?
|
Did we decide to explicitly write |
|
@tech4him1 we haven’t found any good way to do that, other than remaining on CSS modules. We have some global class styles as well for various reasons, so simply prefixing every class wouldn’t work, even if I knew of a tool that did that for us. |
|
@Benaiah I'm thinking:
If anyone really feels this is the wrong direction, please comment, but otherwise we need to get this ready and merged ahead of UI implementations. |
|
@erquhart sounds good to me; I'll try to get that done & pushed today. |
Benaiah
changed the title
|
Note that this is currently WIP - while the styles should be working, I may need to clean up the output of the script I used to replace prefixer calls, and I still need to actually remove the prefixer and the lines which import it. |
Benaiah
force-pushed
the
migrate-to-plain-css
branch
from
1159240
to
240a00f
Compare
| @@ -1,16 +1,16 @@ | |||
| @import "../UI/theme.css"; | |||
|
|
|||
| .input { | |||
| .nc-fileControlinput { | |||
There was a problem hiding this comment.
Thanks, I'll get this fixed.
src/components/stories/Card.js
Outdated
| @@ -37,6 +37,6 @@ storiesOf('Card', module) | |||
| <img src="http://www.top13.net/wp-content/uploads/2015/10/perfectly-timed-funny-cat-pictures-5.jpg" /> | |||
| <h1>Now with footer.</h1> | |||
| <p>header and footer elements are also not subject to margin</p> | |||
| <footer style={styles.footer}>© Thousand Cats Corp</footer> | |||
| <footer style={styles("footer")}>© Thousand Cats Corp</footer> | |||
There was a problem hiding this comment.
should this be a class in place of style?
There was a problem hiding this comment.
Actually, this should stay the way it was before, and it got changed to this when changing the prefixer to a function. In this case the styles object is a manually constructed object earlier in the file, and has nothing to do with the prefixer or CSS modules. I'll change this back.
| @@ -31,12 +31,12 @@ export default class ControlPane extends Component { | |||
| const value = entry.getIn(['data', fieldName]); | |||
| const metadata = fieldsMetaData.get(fieldName); | |||
| const errors = fieldsErrors.get(fieldName); | |||
| const labelClass = errors ? styles.labelWithError : styles.label; | |||
| const labelClass = errors ? `nc-controlPane-label nc-controlPane-labelWithError` : 'nc-controlPane-label'; | |||
There was a problem hiding this comment.
Quotes here should be fixed.
| import stickyStyles from '../UI/Sticky/Sticky.css'; | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Remove extra blank lines from edited JS files.
| @@ -4,7 +4,7 @@ import { Map, fromJS } from 'immutable'; | |||
| import ImmutablePropTypes from 'react-immutable-proptypes'; | |||
| import { resolveWidget } from '../Widgets'; | |||
| import ControlHOC from '../Widgets/ControlHOC'; | |||
| import styles from './ControlPane.css'; | |||
|
|
|||
There was a problem hiding this comment.
Remove extra blank lines from edited JS files.
| @@ -8,7 +8,7 @@ import AppBar from "react-toolbox/lib/app_bar"; | |||
| import FontIcon from "react-toolbox/lib/font_icon"; | |||
| import FindBar from "../FindBar/FindBar"; | |||
| import { stringToRGB } from "../../lib/textHelper"; | |||
| import styles from "./AppHeader.css"; | |||
|
|
|||
There was a problem hiding this comment.
Remove extra blank lines from edited JS files.
| @@ -4,7 +4,7 @@ import Input from "react-toolbox/lib/input"; | |||
| import Button from "react-toolbox/lib/button"; | |||
| import { Card, Icon } from "../../components/UI"; | |||
| import logo from "../git-gateway/netlify_logo.svg"; | |||
| import styles from "../git-gateway/AuthenticationPage.css"; | |||
|
|
|||
There was a problem hiding this comment.
Remove extra blank lines from edited JS files.
| @@ -11,7 +11,7 @@ import { MARK_COMPONENTS, NODE_COMPONENTS } from './components'; | |||
| import RULES from './rules'; | |||
| import plugins, { EditListConfigured } from './plugins'; | |||
| import onKeyDown from './keys'; | |||
| import styles from './index.css'; | |||
|
|
|||
There was a problem hiding this comment.
Remove extra blank lines from edited JS files.
| @@ -2,8 +2,7 @@ import PropTypes from 'prop-types'; | |||
| import React, { Component } from 'react'; | |||
| import { Map } from 'immutable'; | |||
| import { resolveWidget } from '../Widgets'; | |||
| import controlStyles from '../ControlPanel/ControlPane.css'; | |||
| import styles from './ObjectControl.css'; | |||
|
|
|||
There was a problem hiding this comment.
Remove extra blank lines from edited JS files.
src/containers/App.js
Outdated
| @@ -21,8 +21,7 @@ import AppHeader from '../components/AppHeader/AppHeader'; | |||
| import { Loader, Toast } from '../components/UI/index'; | |||
| import { getCollectionUrl, getNewEntryUrl } from '../lib/urlHelper'; | |||
| import { SIMPLE, EDITORIAL_WORKFLOW } from '../constants/publishModes'; | |||
| import styles from './App.css'; | |||
| import sidebarStyles from './Sidebar.css'; | |||
|
|
|||
There was a problem hiding this comment.
Remove extra blank lines from edited JS files.
src/containers/CollectionPage.js
Outdated
| @@ -6,7 +6,7 @@ import { loadEntries } from '../actions/entries'; | |||
| import { selectEntries } from '../reducers'; | |||
| import { Loader } from '../components/UI'; | |||
| import EntryListing from '../components/EntryListing/EntryListing'; | |||
| import styles from "./CollectionPage.css"; | |||
|
|
|||
There was a problem hiding this comment.
Remove extra blank lines from edited JS files.
src/containers/Sidebar.js
Outdated
| @@ -4,7 +4,7 @@ import { connect } from 'react-redux'; | |||
| import ReactSidebar from 'react-sidebar'; | |||
| import _ from 'lodash'; | |||
| import { openSidebar } from '../actions/globalUI'; | |||
| import styles from './Sidebar.css'; | |||
|
|
|||
There was a problem hiding this comment.
Remove extra blank lines from edited JS files.
Benaiah
force-pushed
the
migrate-to-plain-css
branch
from
219308d
to
01e0410
Compare
Benaiah
changed the title
Benaiah
force-pushed
the
migrate-to-plain-css
branch
from
01e0410
to
028f0c0
Compare
| @@ -1,31 +1,31 @@ | |||
| .root { | |||
| .nc-gitGatewayAuthenticationPage-root { | |||
There was a problem hiding this comment.
After this is merged, we need to follow up and condense duplicate styles, such as those shared between the git gateway and github auth pages. Also, classnames need to be structured subject-first, so we'd have nc-auth styles that apply to all auth page implementations, and then nc-auth-git-gateway or nc-auth-github for specifics.
There was a problem hiding this comment.
Agreed. Those were my thoughts as well, I just figured they were out of scope for this PR.
| margin-left: 2%; | ||
| } | ||
|
|
||
| .homeLink .icon { | ||
| .nc-appHeader-homeLink &icon { |
| @@ -1,61 +1,62 @@ | |||
| @import "../UI/theme"; | |||
| @import '../UI/theme.css'; | |||
There was a problem hiding this comment.
This can be removed, theme.css should simply be imported in the master file before this.
| @@ -1,6 +1,6 @@ | |||
| @import '../UI/theme'; | |||
| @import '../UI/theme.css'; | |||
There was a problem hiding this comment.
Thanks, for some reason I had thought this was necessary to get the theme variables in this file. Reading the docs on it that's clearly not the case - not sure where I got that idea,
| @@ -1,39 +1,39 @@ | |||
| @import '../UI/theme'; | |||
| @import '../UI/theme.css'; | |||
| @@ -9,7 +9,6 @@ import ControlPane from '../ControlPanel/ControlPane'; | |||
| import PreviewPane from '../PreviewPane/PreviewPane'; | |||
| import Toolbar from './EntryEditorToolbar'; | |||
| import { StickyContext } from '../UI/Sticky/Sticky'; | |||
| import styles from './EntryEditor.css'; | |||
| import stickyStyles from '../UI/Sticky/Sticky.css'; | |||
Change `prefixer` to a function instead of a proxy
erquhart
force-pushed
the
migrate-to-plain-css
branch
from
7e9103d
to
62895d3
Compare
* Migrate to plain CSS (remove CSS modules) Change `prefixer` to a function instead of a proxy * Switch prefix to `nc` * Replace prefixer with literal class names * Remove prefixer * Fix migration errors * fix compose migrations * Remove unnecessary theme imports * Remove old CSS import * fix sticky toolbar positioning * update to cssnano v4 so preset is used * fix css pseudo selectors * update lockfile
- Summary
Migrate to plain CSS and remove CSS modules. Closes #418.
- Test plan
Manually tested visual elements; I would really appreciate help checking all the nooks and crannies of the UI. Most CSS should be exactly equivalent but there's always the chance I missed something.
- Description for the changelog
Migrate to plain CSS and remove CSS modules.
- A picture of a cute animal (not mandatory but encouraged)