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

Remove Sass from custom CSS files #1450

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

javiereguiluz
Copy link
Member

This is mentioned by @weaverryan in #1449.

We don't really use any Sass feature (except nesting and some variables). I've replaced Sass variables by CSS custom properties ... and removed the nesting by flattening all styles (we can readd nesting soon when native support is a big bigger than today: https://caniuse.com/?search=nesting).

Ryan, if you agree on this, can we just merge it and then you rebase your PR? Thanks!

@@ -50,18 +50,45 @@
@include lato-include-font('normal');
@include lato-include-font('bold');

:root {
--font-heading: 2.5rem;
Copy link
Member

Choose a reason for hiding this comment

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

I would call these --font-size-heading and so on. font-heading makes me think this is expected to define the font used in headings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with you. The name is "bad". But I kept it to use the exact same name as the Sass version, to ease the upgrade 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

I've merged this with the bad variable names ... when the whole upgrade to AssetMapper is finished, we can rename and reorganize these properties. Thanks!

@weaverryan
Copy link
Member

weaverryan commented Nov 8, 2023

Oh, this would make life MUCH easier over on my PR - thank you!

After this, on that PR, we could focus on:

A) importing CSS versions (instead of sass) for all of the vendor packages other than Bootstrap (bootstrap-tagsinput, ~lato-font/scss/public-api, etc). In theory (?) we may be overriding some Sass variables on those. But if we are, we can check on what we're tweaking and find a different solution.
B) Keep Bootstrap - but point at the vendor/ directory. Or do you think we could remove the Bootstrap Sass entirely? We're overriding several things in _variables.scss. What I don't know is how feasible it is to make these same changes by overriding CSS properties... especially since the demo uses Bootstrap 4 at the moment (not sure how many CSS properties they use/expose in v4)

@javiereguiluz javiereguiluz merged commit 8cb63a5 into symfony:main Nov 9, 2023
@javiereguiluz javiereguiluz deleted the remove_sass branch November 9, 2023 16:43
javiereguiluz added a commit that referenced this pull request Dec 11, 2023
This PR was squashed before being merged into the main branch.

Discussion
----------

[WIP] Upgrading to AssetMapper 6.4

Hi!

This converts from Encore to AssetMapper 6.4 (and also upgrading to Symfony 6.4 in general). This was the final product of a live stream walking through this process - https://www.youtube.com/watch?v=pyj1hvhcxGc

There are still some TODOs to finish. The biggest is the first, as Sass is quite embedded in there.

- [X] ~~A) convert Sass->CSS (not a requirement for AssetMapper, but Javier & I talked about this). Then remove SassBundle & bootstrap in `composer.json`. Probably many Sass variables should be converted to CSS variables. See #1450 and #1452~~. Much of this was done, Sass kept just for Bootstrap.
- [X] ~~B) In `assets/app.js`, I should just be able to import from the individual `bootstrap` package instead of requiring each part~~ It's fine how it is. When we upgrade, perhaps we could look at it then.
- [X] C) commit `public/assets/` or `assets/vendor/` so the code works after clone with ZERO other commands. Else, the user will need to run `importmap:install`.
- [x] D) update `devcontainer.json`
- [x] E) fix the `admin` entrypoint & layout

It's a busy month! If anyone is interested in trying to fix any of the above points, just let me know here or on Slack. I'd be more than happy to coach someone through them.

Cheers!

Commits
-------

634f238 [WIP] Upgrading to AssetMapper 6.4
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.

3 participants