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

Setup base of the app with CRA-v2, CSS Modules and Sass #1

Merged
merged 8 commits into from
Oct 1, 2018

Conversation

ka1130
Copy link
Owner

@ka1130 ka1130 commented Oct 1, 2018

This PR sets up the base of the application with CRA-v2 that has CSS Modules and Sass already integrated. Thus, the app has not been yet ejected.

The default view has been changed to a sample header.

To keep the code clean, VSCode prettier plugin has been added with a basic configuration.

@ka1130 ka1130 self-assigned this Oct 1, 2018
@ka1130 ka1130 added the For Review Feature ready for review label Oct 1, 2018
@ka1130 ka1130 requested a review from yakato October 1, 2018 16:13
README.md Outdated
- `yarn install` to install dependencies
- `yarn start` to run the server on `http://localhost:3000`

Demo app to be find [here](https://ka1130.github.io/TVloVe/).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that this shouldn't be here yet

@@ -0,0 +1,13 @@
import React from 'react';
import styles from './styles.module.scss';
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can change the name of scss file to name of the component. It will produce you better className for debugging.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Wow, that's cool!

@@ -0,0 +1,5 @@
body {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add come common resets here, please

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sure, done. Hope I haven't added too much and it is fine not to add reset in a separate file in this case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's enough to have it in this file. What about links text-decoration?

@ka1130 ka1130 force-pushed the cra-css-modules-sass branch from 0b8e192 to 1e4ae30 Compare October 1, 2018 16:46
@ka1130
Copy link
Owner Author

ka1130 commented Oct 1, 2018

Thanks, @yakato ! The trick with changing the naming convention of the style files is awesome, it's a shame I haven't used it before. Yes, I have copy-pasted the contents of README.md from another project, I've corrected it. I've also introduced the meyerweb CSS reset: please advice if this was a good move.

@yakato
Copy link
Collaborator

yakato commented Oct 1, 2018

Thanks for improvements. Perfect basic setup 👍
Re-consider resets as I commented and feel free to merge if it's ok for you.

@ka1130
Copy link
Owner Author

ka1130 commented Oct 1, 2018

Oh, right, text-decoration should be reset too, thanks! :) Cool, I will add this and merge then. :)

@ka1130 ka1130 merged commit 7f40787 into master Oct 1, 2018
@ka1130 ka1130 deleted the cra-css-modules-sass branch October 1, 2018 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Review Feature ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants