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

css transformed into scss #110

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

css transformed into scss #110

wants to merge 1 commit into from

Conversation

mk-sdev
Copy link

@mk-sdev mk-sdev commented Sep 15, 2022

I changed css files into scss files and used watch sass extension which automatically created dist folder with css files. I also changed paths to css files in html files.

@gbowne1
Copy link
Owner

gbowne1 commented Sep 15, 2022

I am still not sure how many of us know how to do SCSS.. so, won't merge at the moment. I will wait for further comment & review @mkaletaa

I have changes going out in a PR today too.

@gbowne1 gbowne1 added bug Something isn't working documentation Improvements or additions to documentation duplicate This issue or pull request already exists help wanted Extra attention is needed good first issue Good for newcomers question Further information is requested labels Sep 15, 2022
@gbowne1 gbowne1 added this to the Website Frontend milestone Sep 15, 2022
@dk3775
Copy link
Collaborator

dk3775 commented Sep 16, 2022

Yea I totally agree with @gbowne1 not many people out here will be knowing about SCSS, so I suggest that we wait for people's review.

@gbowne1
Copy link
Owner

gbowne1 commented Sep 16, 2022

SCSS is not impossible, but since the project already uses compiled SCSS coming from Bootstrap 5.2 CDN, it's a fair assumption we should target this at some point, so that the 1.0 release does not mix style types between CSS and SCSS. IMHO, SCSS can be a much cleaner option.

I saw a comment in one of the files someone put there, not to make a huge styles.css. Do we need to modularize the styles into seperate stylesheets for each page?

As for SCSS, It's not for new people, but it's not impossible and there are tutorials on SCSS.

We would have to test compile times at the browser, and we are already at 1.0 - 1.2 seconds at least according to Google Lighthouse.

Edit: SASS/SCSS tutorial: https://www.w3schools.com/sass/

@Pathholder1806
Copy link
Collaborator

Pathholder1806 commented Sep 16, 2022

Hey @mkaletaa !
It is really good to have you contribute to this and kudos to you for having the changes done.

I would like to add a little suggestion
As per my existing knowledge of SCSS & SAAS. There is a scope of more refactoring in the SCSS files. For eg

One of your changes states:

.mode__toggle:hover {
  transition: .1s;
  width: 42px;
  height: 42px;
}

But with SCSS we can take it like:

.mode__toggle {
  &:hover {
    transition: .1s;
    width: 42px;
    height: 42px;  
  }
}

Now what is the advantage of the above refactoring ?
For eg we have some code like

.enlarge {
  font-size: 14px;
  transition-property: font-size;
  transition-duration: 4s;
  transition-delay: 2s;
}
.enlarge:hover {
  font-size: 36px;
}

We can club both the above blocks into one like:

.enlarge {
  font-size: 14px;
  transition: {
    property: font-size;
    duration: 4s;
    delay: 2s;
  }

  &:hover { font-size: 36px; }
}

Reference: SCSS Docs
And likewise there are other instances also of parent-child relationship in the old CSS file which can be refactored using SCSS
So I guess at this stage still we are not fully exploiting the advantages of SCSS for a cleaner code.
I am also not a master with it so please feel free to add your review to it also.

@gbowne1
Copy link
Owner

gbowne1 commented Sep 16, 2022

@Pathholder1806

SASS/SCSS migration can be done at almost any point. I like your points. It may take a few of us to get it right. I just feel like we should not be mixing style types, i.e. SCSS and CSS. Because Bootstrap CDN sends out SCSS, I feel it may benefit from using a single type, not both.

Using CDN's would keep the end user from having to redownload the new version of libraries, unless we provide bash shell scripts or powershell scripts to download the new versions. Using CDN's would mean

There are CSS to SASS/SCSS conversion tools out there that are great too.

I feel like we could all work on this change.. even if its placed in its own branch and migrated when its done and tested.

@gbowne1 gbowne1 mentioned this pull request Sep 16, 2022
3 tasks
Copy link
Collaborator

@pranavmadhu01 pranavmadhu01 left a comment

Choose a reason for hiding this comment

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

conflicting changes must be resolved

@gbowne1
Copy link
Owner

gbowne1 commented Jan 26, 2023

I am not sure what to do about this. I looked at the files but did not see anything that I thought may be wrong. @pranavmadhu01

@pranavmadhu01
Copy link
Collaborator

the branch has merge conflicts . It should be resolved before merging

@gbowne1
Copy link
Owner

gbowne1 commented Jan 28, 2023

Am aware this needs fixing. I will look at it this weekend.

@mkaletaa @pranavmadhu01

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation duplicate This issue or pull request already exists good first issue Good for newcomers help wanted Extra attention is needed question Further information is requested
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

5 participants