Shopify / dawn

Shopify's first source available reference theme, with Online Store 2.0 features and performance built-in.
Other
2.47k stars 3.31k forks source link

Leave REM unit at default #883

Open eloyesp opened 2 years ago

eloyesp commented 2 years ago

Currently, the down theme use the known 62.5% trick to have a REM unit of 10px, that allows easier calculations, as we are known to be more confortable with those numbers.

While that might be true for most people, the main problem I see with that on a baseline theme, is that it changes a very well known and trusted default, and is almost impossible to change once it is there.

Then, the problems are that, for example: copypasted css is broken when it use rem units, derived themes are forced to inherit the same default and so on.

Seeing the 62.5% trick through the lenses of the main guidelines on the README:

Proposal

So I propose a big search and replace commit, that change all the REM units (I think there is a script doing that on a gist) and change all the divide: 10 to 16 on liquid statements.

We can add some rounding to make the source code look nice, but there will be no functionality impact on the theme itself.

The main issue, is that there are already other themes that are based on this, and they will be broken if they depend on this... so there should be a warning for those cases, but that will be get harder once more people depend on this.

amoser67 commented 2 years ago

In addition to the font sizes, I think that a lot of the css would need to be reworked as well since REM units are used so frequently for margin and padding. Otherwise, the font size would be staying the same, while all of the spacing increases by 60%.

I think one other benefit of 10px = REM is that it minimizes the number of digits in the CSS values. For example, you can represent 1 - 100px with just two digits (.1rem, .2rem, ... 5rem, 5.1rem, ... , 10rem), which isn't possible with 16.

In the end, I agree that 10px = REM feels wrong, and almost like an abuse of REM, however, given how integrated it is into the theme, and the lack of a CSS preprocessor, changing it would make the theme's CSS significantly more difficult to read and edit.

eloyesp commented 2 years ago

In addition to the font sizes, I think that a lot of the css would need to be reworked as well since REM units are used so frequently for margin and padding. Otherwise, the font size would be staying the same, while all of the spacing increases by 60%.

The big commit, is an automated commit that just replace every existing mention of {{ number }}rem by {{ number * 16 / 10 }}rem so there is no real rework except specific points that use calc.

I think one other benefit of 10px = REM is that it minimizes the number of digits in the CSS values. For example, you can represent 1 - 100px with just two digits (.1rem, .2rem, ... 5rem, 5.1rem, ... , 10rem), which isn't possible with 16.

That point is true in part, but the idea is not to use pixels... bootstrap does a lot of magic without using pixels and with 5 values for rems. The problem is that using pixels to think design promotes bad practices, like align pixel sized images with rem sized boxes, that get broken when the user set a non-default font. (see https://stackoverflow.com/questions/28988445/css-62-5-why-do-developers-use-it )

So, we can just round things, and use three meaningful digits at most and have something that is not pixel perfect, but good enough.

In the end, I agree that 10px = REM feels wrong, and almost like an abuse of REM, however, given how integrated it is into the theme, and the lack of a CSS preprocessor, changing it would make the theme's CSS significantly more difficult to read and edit.

I repeat, bootstrap is a good example of a very well known and also very edited (that is, is not difficult to read or to edit), that do not depend on that trick.


That said, I understand that it is an uncomfortable change to do, but is there any good alternative? The main issue is that it makes it hard to share code between projects, Down cannot steal code from bootstrap without changing all the rem mentions.

Obviously it would have been much easier sooner in the development, but the project is 5 months old, it will be much harder later on the development with more people depending on the code.

eloyesp commented 2 years ago

Hey @ludoboludo @KaichenWang, do you have any feedback on this? this default makes it quite hard to share code between projects and to re-use legacy code from existing themes.

ludoboludo commented 2 years ago

Hey folks 👋

So we're using, as you mentioned, the percentage to set the font-size on the HTML element and this way 1rem=10px.

But it's not always this value as we've added a setting for people to change the font size and make it bigger if they feel the need to. So now we have a calc() function in the CSS to change the base value.

It will impact anything using rem on the theme including spacing, etc. We've set a few things back to pixel values for that reason. Button sizes for example, as we want to keep the 44px by 44px sizing for accessibility purposes.

One benefit of using a percentage as well is that it's based on the browser settings and respecting the user's preference. So if I was to set my browser to be using a small font size, the font size of the theme would scale based on it.

But if we had set 10px, then it wouldn't match the user's preference, as we are setting a set value rather than relative one. Outcome example screenshot

@eloyesp is the main concern that it can creates issues when porting some of the code onto another theme/project that might not use the same approach? Because for now in a theme environment we feel that it's a good approach that respect user preferences and is easy to read.

Though I think I'm missing a bit the explanation on how bootstrap does it 🤔 or what you would edit in the current code to make it work better/easier to share. So if you don't mind giving me an example of what you'd change on our current code base that would be great 🙂

eloyesp commented 2 years ago

Though I think I'm missing a bit the explanation on how bootstrap does it (...)

Bootstrap do not do anything special, it just live with rem as a unit, not linking it to px.

is the main concern that it can creates issues when porting some of the code onto another theme/project

Yes, it is the main concern, but it is not the only concern, and it runs mainly on the other direction... because with this non-default value for rem, every other library that use rem units get broken, so the problem is that you isolate your theme making it harder to use other code here.

The other problem, is that it promotes thinking on pixels, making it easier to build experiences that break when the user use a non standard font-size value, so it fuels a bad practice.

So if you don't mind giving me an example of what you'd change on our current code base that would be great

I've just made a draft PR to show how it would be read the code after the change.

Nedi11 commented 1 year ago

This has still not been fixed, +1 for using defualt font size at root

willwongco commented 2 months ago

+1 here - the 62.5% trick makes it very difficult to make the Dawn theme work with tailwindcss

eloyesp commented 2 months ago

Yes, for this and other reasons Dawn is a very bad theme to use as a base theme for development. I'm not saying that Dawn is a bad theme, it is just not good as a base theme.

If anyone knows a good base theme for shopify or is working on something like that, it would be really nice to mention it here.

tnoetzel commented 1 month ago

Also find this extremely annoying!

That said, I did find a partial workaround... this plugin converts Tailwind rem units to pixels. Usage wasn't obvious from the readme, but I configured my tailwind.config.js file like so:

    plugins: [
        remToPxPlugin({
            baseFontSize: 16,
        })
    ],

Not a real solution, but better than nothing if you want to use the standard tailwind sizing for everything not directly tied to dawn.

tnoetzel commented 1 month ago

Yes, for this and other reasons Dawn is a very bad theme to use as a base theme for development. I'm not saying that Dawn is a bad theme, it is just not good as a base theme.

If anyone knows a good base theme for shopify or is working on something like that, it would be really nice to mention it here.

@eloyesp - Curious what solution/approach you ended up with

eloyesp commented 1 month ago

@tnoetzel I don't have a solution ready, but my approach would be to build a simplified theme, with a similar UI, but a simplified markup (without unnecessary nesting), using tailwind (without any css) and with a very small javascript. That is less features than Dawn, but a much shorter/readable code.

One important thing, is that Dawn offers a lot of customization options for the end user, that is great for the end user, but it is a nightmare to make custom developments over, so the main change would be to have near zero options/settings.

I'm working on that now, and will open source if I reach something usable.