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

Refactor styling (split sources to assets, use css variables for colors....) #98

Closed
wants to merge 1 commit into from

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Feb 27, 2023

  • Drop dead code (pretty much was never used by the theme)
  • Use css variables for colors to allow easier theming
  • Split code so it is better maintainable
    • but still provide just one CSS file for performance (all scss files are compiled to one CSS by hugo)

@zjedi
Copy link
Owner

zjedi commented Feb 27, 2023

Looks pretty useful. I've got a site with some CSS customization and it would have been much easier having it split like this.

@zjedi

This comment was marked as resolved.

@susnux
Copy link
Contributor Author

susnux commented Mar 8, 2023

Currently support is about 96% of global users: https://caniuse.com/css-variables

If you want to also support those 4% of users:

  1. Add a default fallback
  2. Or use a ponyfill like https://github.com/jhildenbiddle/css-vars-ponyfill for legacy browsers.

@zjedi
Copy link
Owner

zjedi commented Mar 8, 2023

Hmm, 4% is not totally negligible, though it should improve over time. We should look into fallback options.

@zjedi zjedi mentioned this pull request Mar 12, 2023
Copy link
Owner

Choose a reason for hiding this comment

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

I'm afraid splitting the css this way (delete original and createy multiple new files) is going to cause unnecessary trouble for those having a site already and pulling new code from master. I'll try to refactor in similar fashion, but keeping the git history, which should help to migrate any potential forked customizations

Copy link
Owner

@zjedi zjedi left a comment

Choose a reason for hiding this comment

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

The stuff under base folder seems a bit too fragmented, I'm planning to put it all together in something like content.scss or structure.scss.

@susnux susnux force-pushed the feat/themeable branch 2 times, most recently from d32f392 to 7285a44 Compare April 1, 2023 04:00
* Drop dead code
* Use color variables to allow easier theming

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux
Copy link
Contributor Author

susnux commented Apr 1, 2023

I rebased the code and added that ponyfill to also support those 4%.

I'm afraid splitting the css this way (delete original and createy multiple new files) is going to cause unnecessary trouble for those having a site already and pulling new code from master.

Only if you modified the css files and in that case you will get a rebase issue so you can adjust the scss instead :)

@zjedi
Copy link
Owner

zjedi commented Apr 1, 2023

I rebased the code and added that ponyfill to also support those 4%.

@susnux I was playing with this branch's concepts and it looks like scss is compiled into css, so in fact you weren't using the real CSS variables that have a risk of being unsupported. --- The scss variables are replaced at compile time, right? Is the ponyfill workaround actually useful somehow?

@susnux
Copy link
Contributor Author

susnux commented Apr 1, 2023

I am using css variables here (note the var(--some-var) syntax).
Those variables are preserved so that users can override them without having to change the theme.

E.g. simply add a custom stylesheet:

:root {
--text-main: blue!important;
}

SCSS variables on the other hand can not be overridden without changing the theme, you would have to change them in the colors.scss file (scss variables look like: $text-main: blue; color: $text-main).
Those would be compiled away and we would not need a ponyfill.

@zjedi
Copy link
Owner

zjedi commented Apr 1, 2023

E.g. simply add a custom stylesheet:

That means the user must include the custom css via custom head as a separate file. I wonder if there's a way in Hugo templates to include such customization and compile it into theme's css in order to better performance (transfer just one file instead of two). It's not a big deal, just trying to figure out the best practice.

Another way I can think of would be to expose scss variable via theme parameters. That seems most comfortable for the users who just want to change the colours. Or maybe we could create a param to specify path to custom css file that would be compiled in. @susnux what do you think?

@zjedi
Copy link
Owner

zjedi commented Apr 1, 2023

@susnux I've transplanted your work on #133, co-authored.

This PR is solid, but there were some conflicts and broken features that were implemented since you opened this. I've played with the code and did some testing, should be ok now. Please check the new PR and let me know if that's ok with you.

Thanks for contributing, I've learned from this.

@zjedi zjedi closed this Apr 1, 2023
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