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

Convert complex CSS files to SCSS #390

Merged
merged 0 commits into from
Aug 18, 2022
Merged

Convert complex CSS files to SCSS #390

merged 0 commits into from
Aug 18, 2022

Conversation

6km
Copy link
Member

@6km 6km commented Jul 20, 2022

Description

Convert complex CSS files to SCSS

Fixes #374

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Locally

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@6km 6km requested review from atapas and koustov as code owners July 20, 2022 01:01
@vercel
Copy link

vercel bot commented Jul 20, 2022

@6km is attempting to deploy a commit to a Personal Account owned by @reactplay on Vercel.

@reactplay first needs to authorize it.

@vercel
Copy link

vercel bot commented Jul 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
react-play ✅ Ready (Inspect) Visit Preview Aug 16, 2022 at 3:36AM (UTC)

@6km
Copy link
Member Author

6km commented Jul 24, 2022

@nirmalkc can you review?

@atapas
Copy link
Member

atapas commented Jul 25, 2022

@6km Please resolve merge conflicts.

@6km
Copy link
Member Author

6km commented Jul 25, 2022

@6km Please resolve merge conflicts.

done

@nirmalkc
Copy link
Collaborator

@6km and @atapas

Here is my recommendation about the SASS file structure. This approach will be much easier to maintain and reuse in the long run.

src/
|
|- base /
| |- _variables.scss // All CSS 4 Variables
| |- _typography.scss // Typography rules
|
|- components /
| |- _buttons.scss // Buttons
| |- _indicators.scss // Spinner, progress bar, preloaders etc.,
|
|– layout/
| |– _navigation.scss // Navigation
| |– _header.scss // Header
| |– _search.scss // Search
|
|– pages/
| |– _home.scss // Home page styles
| |– _playideas.scss // Playideas
| |– _playlist.scss // Contact specific styles
|
|- App.scss

Though we are in SCSS eco-system, lets continue to use CSS4 Variables for any direct exposure of visual properties.

@nirmalkc
Copy link
Collaborator

@atapas , should we continue to rely on Google Font repo for direct font include or shall we keep the fonts in our local git folder, so that we dont rely on them anymore? I am fine with either way, but keeping it in our local repo-reduces the dependency.

@import url('https://fonts.googleapis.com/css2?family=Poppins:wght@300;400;500;600&family=Raleway:wght@900&display=swap');

@atapas
Copy link
Member

atapas commented Jul 29, 2022

@atapas , should we continue to rely on Google Font repo for direct font include or shall we keep the fonts in our local git folder, so that we dont rely on them anymore? I am fine with either way, but keeping it in our local repo-reduces the dependency.

@import url('https://fonts.googleapis.com/css2?family=Poppins:wght@300;400;500;600&family=Raleway:wght@900&display=swap');

We can keep them in local.

@nirmalkc
Copy link
Collaborator

nirmalkc commented Aug 2, 2022

@6km and @atapas

Here is my recommendation about the SASS file structure. This approach will be much easier to maintain and reuse in the long run.

src/ | |- base / | |- _variables.scss // All CSS 4 Variables | |- _typography.scss // Typography rules | |- components / | |- _buttons.scss // Buttons | |- _indicators.scss // Spinner, progress bar, preloaders etc., | |– layout/ | |– _navigation.scss // Navigation | |– _header.scss // Header | |– _search.scss // Search | |– pages/ | |– _home.scss // Home page styles | |– _playideas.scss // Playideas | |– _playlist.scss // Contact specific styles | |- App.scss

Though we are in SCSS eco-system, lets continue to use CSS4 Variables for any direct exposure of visual properties.

@atapas and @6km have we decided to keep the SCSS files within the respective folders like header, platlist etc.,? If yes, then we are good to merge.

@atapas
Copy link
Member

atapas commented Aug 2, 2022

@6km and @atapas
Here is my recommendation about the SASS file structure. This approach will be much easier to maintain and reuse in the long run.
src/ | |- base / | |- _variables.scss // All CSS 4 Variables | |- _typography.scss // Typography rules | |- components / | |- _buttons.scss // Buttons | |- _indicators.scss // Spinner, progress bar, preloaders etc., | |– layout/ | |– _navigation.scss // Navigation | |– _header.scss // Header | |– _search.scss // Search | |– pages/ | |– _home.scss // Home page styles | |– _playideas.scss // Playideas | |– _playlist.scss // Contact specific styles | |- App.scss
Though we are in SCSS eco-system, lets continue to use CSS4 Variables for any direct exposure of visual properties.

@atapas and @6km have we decided to keep the SCSS files within the respective folders like header, platlist etc.,? If yes, then we are good to merge.

@nirmalkc I would look for suggestions here. What would be best for the long run? Please remember, the plays will go to a separate repository soon. So its better if the respective artefacts stay close by.

@nirmalkc
Copy link
Collaborator

nirmalkc commented Aug 2, 2022

@6km and @atapas
Here is my recommendation about the SASS file structure. This approach will be much easier to maintain and reuse in the long run.
src/ | |- base / | |- _variables.scss // All CSS 4 Variables | |- _typography.scss // Typography rules | |- components / | |- _buttons.scss // Buttons | |- _indicators.scss // Spinner, progress bar, preloaders etc., | |– layout/ | |– _navigation.scss // Navigation | |– _header.scss // Header | |– _search.scss // Search | |– pages/ | |– _home.scss // Home page styles | |– _playideas.scss // Playideas | |– _playlist.scss // Contact specific styles | |- App.scss
Though we are in SCSS eco-system, lets continue to use CSS4 Variables for any direct exposure of visual properties.

@atapas and @6km have we decided to keep the SCSS files within the respective folders like header, platlist etc.,? If yes, then we are good to merge.

@nirmalkc I would look for suggestions here. What would be best for the long run? Please remember, the plays will go to a separate repository soon. So its better if the respective artefacts stay close by.

Play specific scss can be bundled in the same folder. Whereas the platform code can be structured as per my above recommendation, atleast the base, components and layout part.

@nirmalkc
Copy link
Collaborator

nirmalkc commented Aug 2, 2022

@6km , if can you please take care of the file structure changes, I think we are good to merge.

@6km
Copy link
Member Author

6km commented Aug 2, 2022

@6km and @atapas

Here is my recommendation about the SASS file structure. This approach will be much easier to maintain and reuse in the long run.

src/ | |- base / | |- _variables.scss // All CSS 4 Variables | |- _typography.scss // Typography rules | |- components / | |- _buttons.scss // Buttons | |- _indicators.scss // Spinner, progress bar, preloaders etc., | |– layout/ | |– _navigation.scss // Navigation | |– _header.scss // Header | |– _search.scss // Search | |– pages/ | |– _home.scss // Home page styles | |– _playideas.scss // Playideas | |– _playlist.scss // Contact specific styles | |- App.scss

Though we are in SCSS eco-system, lets continue to use CSS4 Variables for any direct exposure of visual properties.

I agree with you, I think that this will be better to maintain and easier to understand for new contributors.

I will work on it ASAP.

@nirmalkc
Copy link
Collaborator

nirmalkc commented Aug 5, 2022

@6km and @atapas
Here is my recommendation about the SASS file structure. This approach will be much easier to maintain and reuse in the long run.
src/ | |- base / | |- _variables.scss // All CSS 4 Variables | |- _typography.scss // Typography rules | |- components / | |- _buttons.scss // Buttons | |- _indicators.scss // Spinner, progress bar, preloaders etc., | |– layout/ | |– _navigation.scss // Navigation | |– _header.scss // Header | |– _search.scss // Search | |– pages/ | |– _home.scss // Home page styles | |– _playideas.scss // Playideas | |– _playlist.scss // Contact specific styles | |- App.scss
Though we are in SCSS eco-system, lets continue to use CSS4 Variables for any direct exposure of visual properties.

I agree with you, I think that this will be better to maintain and easier to understand for new contributors.

I will work on it ASAP.

@6km , can we atleast bring the common areas like the variables, typography, buttons in separate SCSS?

@6km
Copy link
Member Author

6km commented Aug 5, 2022

@6km , can we atleast bring the common areas like the variables, typography, buttons in separate SCSS?

I'm working on the new file structure right now, sorry for being late

@6km 6km added work in progress The work is in progress review ready and removed waiting for reviewers work in progress The work is in progress labels Aug 5, 2022
@6km 6km requested a review from nirmalkc August 5, 2022 21:48
@6km
Copy link
Member Author

6km commented Aug 5, 2022

@nirmalkc I have been working on the new file structure, can you re-review please?

@6km
Copy link
Member Author

6km commented Aug 5, 2022

Btw, I have replaced node-sass with sass as it's currently deprecated.

image

@koustov
Copy link
Member

koustov commented Aug 7, 2022

@nirmalkc can you taka a look?

@atapas
Copy link
Member

atapas commented Aug 9, 2022

@6km resolve merge conflicts

@6km
Copy link
Member Author

6km commented Aug 9, 2022

@6km resolve merge conflicts

done

@6km
Copy link
Member Author

6km commented Aug 11, 2022

can anyone review please?

@atapas
Copy link
Member

atapas commented Aug 13, 2022

can anyone review please?

@6km It's ready. I just want someone to test it well and confirm.

@6km
Copy link
Member Author

6km commented Aug 13, 2022

@6km It's ready. I just want someone to test it well and confirm.

@atapas Great! 👍

@atapas
Copy link
Member

atapas commented Aug 18, 2022

Hi @6km can you please resolve the conflicts?

@vasantisuthar as you are testing it, please provide your updates. We want to merge it asap as the changes will lead to merge conflicts very often.

@6km 6km merged commit 3efb9bc into reactplay:main Aug 18, 2022
@vasantisuthar
Copy link
Contributor

Hi @6km can you please resolve the conflicts?

@vasantisuthar as you are testing it, please provide your updates. We want to merge it asap as the changes will lead to merge conflicts very often.

Will update you tomorrow

@6km
Copy link
Member Author

6km commented Aug 18, 2022

Hi @6km can you please resolve the conflicts?
@vasantisuthar as you are testing it, please provide your updates. We want to merge it asap as the changes will lead to merge conflicts very often.

Will update you tomorrow

I have wrongfully merged this PR with 0 commits, I will open another one tomorrow, please be ready to review it 🙏

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.

✨ [Feature request]: Convert complex CSS files to SCSS
5 participants