Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 77 scss refactor #88

Merged
merged 6 commits into from
Feb 21, 2018
Merged

Issue 77 scss refactor #88

merged 6 commits into from
Feb 21, 2018

Conversation

andrew-c-tran
Copy link
Contributor

@andrew-c-tran andrew-c-tran commented Feb 15, 2018

The Problem/Issue/Bug:

We were using a global css file vs modular SCSS

How this PR Solves The Problem:

overrides.css (former global stylesheet) was broken down to separate SCSS files, and each component calls it's associated style. For example, all styles for the project cards are in cards.scss, and is called by the javascript component (site-cards.js)

Manual Testing Instructions:

make clean && make npmstart will clear node_modules, reinstall via npm install, run webpack to bundle assets, and finally launch the development version of the app via electron.

Project should remain unchanged visually as no CSS rules were added or removed in the conversion process.

Related Issue Link(s):

#77

…m associated views. TODO: comment SCSS, replace all colors with variables.
@andrew-c-tran
Copy link
Contributor Author

andrew-c-tran commented Feb 15, 2018

WIP - need to comment SCSS still. Functionality is complete.

@andrew-c-tran
Copy link
Contributor Author

One thing that is needed is a loading spinner or some sort of indicator for the UI until the application is initialized. Currently, there is a grey screen as UI.JS is loaded. This is a subsecond load, but in the event that something fails, the user will be staring at a grey screen. Some design thought needs to be put into how we are going to display the loading screen and relay any startup sequence failures. I believe this should be addressed in #42

test: /\.(eot|svg|ttf|woff|woff2)$/,
loader: 'file-loader?name=/fonts/[name].[ext]',
},
/* {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're going to comment this out and it's meant to remain this way, please make sure to add a comment about why you're commenting it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, you're right, that should be removed.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a perfectly fine piece of work to me. It seems we'll want to move a few more things into variables as time goes on, but this seems like a good path. Thanks!

@andrew-c-tran andrew-c-tran merged commit aa69ebf into ddev:master Feb 21, 2018
@rickmanelius rickmanelius mentioned this pull request Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants