Closed robsonsobral closed 3 years ago
I'm also trying to remove the @imports
as they're on their last breath.
This looks great. Thank’s so much @robsonsobral
@scottkellum , hi! Bom dia!
What do you think of the suggestion on the last commit? It is far too common for me that designers use 90px instead 92px, as our previous agreement about the modular scale.
(Also, it's easier to write the target size, than keep looking at a table to find the right step.)
Oooh wow I really love this idea! This is fantastic
@robsonsobral Sorry I haven’t been very available on this lately. If you ever want to have a call to regroup on this project feel free to book some time here: https://calendly.com/scottkellum
Hi, @scottkellum . I've been working on late Brazilian hours (Also, I feel intimidated to have a call with so important dev.)
I've been testing the lib in parallel on a project I'm working on and fixing it. If you can give me more time, I'll put more effort on the lib.
Yeah no rush I really appreciate what you are doing with this.
Sorry for the silence, @scottkellum !
I've been testing the step(16px)
and getting furious I didn't think about it earlier! It soooo useful!
I set stylelint following SASS guidelines on the repo to make future collaboration from the community easier.
The way settings are loaded isn't flexible enough for day-to-day work. An example from tests:
The documentation of Sass has a suggestion for it:
Configuring modules with @use ... with can be very handy, especially when using libraries that were originally written to work with the @import rule. But it’s not particularly flexible, and we don’t recommend it for more advanced use-cases. If you find yourself wanting to configure many variables at once, pass maps as configuration, or update the configuration after the module is loaded, consider writing a mixin to set your variables instead and another mixin to inject your styles.
// _library.scss
$-black: #000;
$-border-radius: 0.25rem;
$-box-shadow: null;
/// If the user has configured `$-box-shadow`, returns their configured value.
/// Otherwise returns a value derived from `$-black`.
@function -box-shadow() {
@return $-box-shadow or (0 0.5rem 1rem rgba($-black, 0.15));
}
@mixin configure($black: null, $border-radius: null, $box-shadow: null) {
@if $black {
$-black: $black !global;
}
@if $border-radius {
$-border-radius: $border-radius !global;
}
@if $box-shadow {
$-box-shadow: $box-shadow !global;
}
}
@mixin styles {
code {
border-radius: $-border-radius;
box-shadow: -box-shadow();
}
}
// style.scss
@use 'library';
@include library.configure(
$black: #222,
$border-radius: 0.1rem
);
@include library.styles;
As this would be a breaking change, I think it's better to change it before release.
I'm sorry it has been taking so long.
@robsonsobral this is really fantastic thank you so much! And open source without financial backing always takes time so no worries about it taking a while.
I’ll dig in when I have more bandwidth but I really like where you have taken this.
While I try to figure out what awesome things ( 😳 ) you mentioned on twitter, @scottkellum , could you tell me what you think of rename the step
mixin? respond-to
, maybe? I guess to avoid homonymous can make the use easier.
I added the possibility to use the syntax 36 at 5
, with spaces, to make it easier to read.
Also, the named ratios can now be used as a simple string, instead of a prefixed reference:
($base: 1em, $ratio: fifth)
The override of settings is my next step.
I'm looking at my Bringhurst, Muller-Brochmann and Tschichold books for something that could be useful.
Thank you!
OK! Finally, we have 100% of code coverage!
The use of git diff for tests is compromised by the unique-id ()
used for Typetura.
Settings can be override after initial loading by the use of the mixin configure($new-settings)
.
Should the override of settings reset the value of respond
? Currently, it does.
The responsive mixin could allow inline settings like the step()
function.
Do you mind of a ratios
key within $settings
map?
I have some thoughts yet about the responsive part.
Thanks so much @robsonsobral ! Getting the target stuff in here like step(16px)
is fantastic. Along with the modernization and testing coverage you have done. I really appreciate it and it makes modular scale better. That’s what I meant by my tweet 😅.
I'm going to merge into my 4.x branch to better play with what you have and respond to your questions.
@robsonsobral a few issues here.
True was not resolving for me initially. I had to make the path be @use '../node_modules/sass-true/sass/true'
Two issues with setup:
$settings
nested inside of the settings feels redundant. Let's look into merging $settings
up a level so you can just type @use 'modularscale' as ms with (base: 1.2em, ratio: 1.33);
. Here is a branch where I was poking around: https://github.com/modularscale/modularscale-sass/pulls
Oh, I totally agree on the settings. I've detected their redundancy. However, I wasn't sure I made it more evident or created it as I saw @base, $ratio, $settings
somewhere. That was something I wanted to ask you about. So, as the breakpoints without a parent property.
On the Sass-True, it's weird it doesn't load. Maybe my Sass installation is outdated. I gonna check it out.
Also, I gonna take a look at default settings. I saw it so many times on your code, that maybe I got confuse. I'm sorry.
@robsonsobral all good thank you for your work on this!
I'll look at my sass version as well
I updated from "1.34.0" to "1.42.1".
If you want, we can work together on code. I promise to do a git pull --rebase
before every git push
🙏🏼
@scottkellum , may I follow this path? What do you think? Does it help?