The Wayback Machine - https://web.archive.org/web/20221231235537/https://github.com/twbs/bootstrap/pull/37398
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

Utilities functions mixin #37398

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Utilities functions mixin #37398

wants to merge 9 commits into from

Conversation

mdo
Copy link
Member

@mdo mdo commented Oct 30, 2022

Replaces #37114 because I couldn't push to that PR.

Re-opening of #35977 after accidentally closing it on my repo (sorry again for that). @mdo, I hope I didn't lose any of your edits in the process as I've also rebased main to fix conflicts in the docs.

As a reminder of what it was about:

Code for #35945. Just threw some code in to get the shapes of the functions. Still need testing, handling all the cases (values not being maps for example), and docs updates (but I'll do that last once the code is set).
The PR adds the following:

  1. functions to get options, a specific option or a specific value for a given utility:
    a. utilities-get-options
    b. utilities-get-option
    c. utilities-get-value
  2. mixins to set options (in bulk or a specific one), new values or remove values for a given utility:
    a. utilities-set-options (defaults to merging with existing, but can be used to completely override by passing $merge: false, allowing to completely redefine a utility if necessary)
    b. utilities-set-option (just a thin wrapper on utilities-set-options)
    c. utilities-add-values
    d. utilities-remove-values
  3. mixins to add or remove utilities (they don't do as much as the rest, but provide consistency)
    a. utilities-add
    b. utilities-remove

Please let me know if there'd be one I missed, or if some are too much (2.b, 3.a. and 3.b for example only bring a consistent API by thinly wrapping other existing functions/mixins)

@mdo
Copy link
Member Author

mdo commented Oct 31, 2022

@julien-deramond @louismaximepiton @ffoodd Would love your thoughts on this! Kind of a few PRs worth of history to look through for the full context, but would love to add this for v5.3.

@romaricpascal
Copy link
Contributor

romaricpascal commented Nov 1, 2022

Happy to answer any question about the code 😄 One of the things buried in the previous PRs (sorry for the mess with my branches) is that the code is checked by a test suite (kept on a separate branch as it'd rely on sass-true to have been brought into the project, which is getting sorted in #36029).

Copy link
Contributor

@louismaximepiton louismaximepiton left a comment

Just few questions, small things to change IMO and as global comments:

Should we introduce default values on each function/mixin ?
Should we check each time for existence of the map ?
What's the future for the maps.scss file ? I thought it was for this kind of purpose (redefine some maps before using them inside utilities API).

However, this sounds really great to have inside Bootstrap. This doesn't add compiled CSS, might not bring any breaking change, might be really useful when well used ! Great work :)

scss/mixins/_utilities.scss Show resolved Hide resolved
@return $values;
}

@function map-from($list-or-string) {
Copy link
Contributor

@louismaximepiton louismaximepiton Nov 8, 2022

Choose a reason for hiding this comment

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

Just to be sure if I understand well: if we use a string here, there should be only 1 item in the map right ? or is there a way to do multiple item map from a string ?

Copy link
Contributor

@romaricpascal romaricpascal Dec 26, 2022

Choose a reason for hiding this comment

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

Yup, @each won't split the string in any way and treat it as a list with 1 item in it.

scss/mixins/_utilities.scss Show resolved Hide resolved
scss/mixins/_utilities.scss Show resolved Hide resolved

// Gets the value of a specific option for a given utility
@function utilities-get-option($utility-name, $option-name) {
$options: utilities-get-options($utility-name);
Copy link
Contributor

@louismaximepiton louismaximepiton Nov 8, 2022

Choose a reason for hiding this comment

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

Since we're experiencing some troubles with some overriding Sass variables, should we use a nomenclature such as $utility-options: ...

}

$utilities: map-merge(
$utilities,
Copy link
Contributor

@louismaximepiton louismaximepiton Nov 8, 2022

Choose a reason for hiding this comment

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

I suppose $utilities cannot and should not be null here ?

Copy link
Contributor

@romaricpascal romaricpascal Dec 26, 2022

Choose a reason for hiding this comment

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

$utilities is the global $utilities variable so is defined by Bootstrap. There's always a risk someone made it null. However, things will blow up in utilities/_api during generation if that's the case, so I'm no sure it's worth being more defensive here.

@mixin utilities-remove-values($utility-name, $value-names...) {
$values: utilities-get-values($utility-name);

$updated-values: map-remove($values, $value-names...);
Copy link
Contributor

@louismaximepiton louismaximepiton Nov 8, 2022

Choose a reason for hiding this comment

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

Should we check for values not to be null ?

Copy link
Contributor

@romaricpascal romaricpascal Dec 26, 2022

Choose a reason for hiding this comment

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

Same as in the previous function, values won't be null thanks to map-from inside utilities-get-values.


// Remove specific values from a given utility
@mixin utilities-remove-values($utility-name, $value-names...) {
$values: utilities-get-values($utility-name);
Copy link
Contributor

@louismaximepiton louismaximepiton Nov 8, 2022

Choose a reason for hiding this comment

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

Should use the same name as the previous function ?


// Add a new utility to the utilities map
@mixin utilities-add($utility-name, $utility) {
$utilities: map-merge(
Copy link
Contributor

@louismaximepiton louismaximepiton Nov 8, 2022

Choose a reason for hiding this comment

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

Should we use @include utilities-set-options($utility-name, $utility, false); ?

site/content/docs/5.2/customize/color.md Show resolved Hide resolved
Copy link
Contributor

@romaricpascal romaricpascal left a comment

@louismaximepiton Thanks for the review 😄 I've finally found some time to provide some answers, at least for those related to the internals of the functions/mixins. I can only let you and the Bootstrap team decide on whether functions should be extracted in other files and such 😄 .

I'm not quite sure how to deal with the few amends as it's now @mdo's branch on the Bootstrap repo. A PR towards this branch feels a bit overkill, and it'll likely be much faster if someone with write access moves things around (especially as I don't have as much capacity to contribute nowadays, sorry).

As a side note, now #36029 has been merged, it's likely worth pulling in the tests I had set up on a separate branch for writing these functions and mixins.

scss/mixins/_utilities.scss Show resolved Hide resolved
@return $values;
}

@function map-from($list-or-string) {
Copy link
Contributor

@romaricpascal romaricpascal Dec 26, 2022

Choose a reason for hiding this comment

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

Yup, @each won't split the string in any way and treat it as a list with 1 item in it.


// Set options for a given utility, merged with the existing ones by default.
// To completely replace the existing options, set the 3rd `$merge` parameter to false
@mixin utilities-set-options($utility-name, $options, $merge: true) {
Copy link
Contributor

@romaricpascal romaricpascal Dec 26, 2022

Choose a reason for hiding this comment

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

Good point on utilities-add being utilities-set-options with $merge: false. I agree it's likely worth refactoring so utilities-add just delegates to utilities-set-options to avoid duplication.

}

$utilities: map-merge(
$utilities,
Copy link
Contributor

@romaricpascal romaricpascal Dec 26, 2022

Choose a reason for hiding this comment

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

$utilities is the global $utilities variable so is defined by Bootstrap. There's always a risk someone made it null. However, things will blow up in utilities/_api during generation if that's the case, so I'm no sure it's worth being more defensive here.

@mixin utilities-remove-values($utility-name, $value-names...) {
$values: utilities-get-values($utility-name);

$updated-values: map-remove($values, $value-names...);
Copy link
Contributor

@romaricpascal romaricpascal Dec 26, 2022

Choose a reason for hiding this comment

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

Same as in the previous function, values won't be null thanks to map-from inside utilities-get-values.

$utility-name,
values,
map-merge(
$existing-values,
Copy link
Contributor

@romaricpascal romaricpascal Dec 26, 2022

Choose a reason for hiding this comment

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

utilities-get-values will return an empty map through map-from, so we can always run map-merge without worry here 😄 That'll create a values field if it didn't exist already, but it feels fair as it fulfils what the function name says.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment