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

Variables update #923

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Conversation

lraycheva
Copy link

I have added prettier so that styles are formatted easily by running "npm run format:fix".

I have updated variables:

  • convert hard-coded colors in style files to variables that are listed in _variables.

I am making this change to easily make theme styling based only on variables override.
Also I noticed that there is no standardized formatting so I installed prettier.

@josdejong
Copy link
Owner

Thanks Luchiya, that looks like a nice improvement with the new scss variables.

Currently there is a linter in place for JavaScript: standardjs. However this doesn't support SCSS. prettier and standardjs may bite each other, it maybe an idea to move to prettier for both JS and SCSS so we have a unified solution. What do you think?

@lraycheva
Copy link
Author

It sounds reasonable.

@josdejong
Copy link
Owner

Ok cool. Do you know what would be needed still to replace standardjs with prettier? It looks like the new format:check checks both js and scss, so it's probably already working?

printWidth: 120,
semi: false,
singleQuote: true,
tabWidth: 4,
Copy link
Owner

Choose a reason for hiding this comment

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

Can you change the tabWidth to 2 spaces?

@josdejong
Copy link
Owner

Thanks for the update @lraycheva, I think we're close. I tried but the JS files aren't yet scanned, there is probably something that need to be changed in the file names glob ./{.,src/**}/*.{css,scss,js}. I tried by simply changing one line in one of the JS files such that it has wrong indentation - prettier should pick this up then. After that there's probably a bit if tuning in the configuration of prettier needed to match the current code style (or we can to decide to change the style, let's see).

Can you also change the indentation to 2 spaces please?

@josdejong
Copy link
Owner

ping @lraycheva

@josdejong
Copy link
Owner

Thanks for fixing the indentation @lraycheva. I see the JS files are still not scanned, did you already have a look into that?

@josdejong
Copy link
Owner

ok thanks for giving it a try. I will give it a try too as soon as I have some time ok? Let's keep this PR open until then, it's probably close to a fully working solution.

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