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

4.x #174

Merged
merged 41 commits into from
Oct 7, 2021
Merged

4.x #174

merged 41 commits into from
Oct 7, 2021

Conversation

robsonsobral
Copy link
Collaborator

@scottkellum , may I follow this path? What do you think? Does it help?

@robsonsobral
Copy link
Collaborator Author

I'm also trying to remove the @imports as they're on their last breath.

@scottkellum
Copy link
Member

This looks great. Thank’s so much @robsonsobral

@robsonsobral
Copy link
Collaborator Author

robsonsobral commented Aug 9, 2021

@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.)

@scottkellum
Copy link
Member

Oooh wow I really love this idea! This is fantastic

@robsonsobral
Copy link
Collaborator Author

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!

@robsonsobral
Copy link
Collaborator Author

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.

@scottkellum
Copy link
Member

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.

@scottkellum scottkellum merged commit e40dfa2 into modularscale:4.x Oct 7, 2021
@scottkellum
Copy link
Member

@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:

  • Default settings do not work so when you aren't passing any settings into the use command it fails.
  • having $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);.

@scottkellum
Copy link
Member

Here is a branch where I was poking around: https://github.com/modularscale/modularscale-sass/pulls

@robsonsobral
Copy link
Collaborator Author

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.

@scottkellum
Copy link
Member

@robsonsobral all good thank you for your work on this!

I'll look at my sass version as well

@robsonsobral
Copy link
Collaborator Author

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🙏🏼

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.

None yet

2 participants