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

Add functions and mixins to manipulate the utlities map #35977

Closed

Conversation

romaricpascal
Copy link
Contributor

@romaricpascal romaricpascal commented Mar 9, 2022

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)

@romaricpascal
Copy link
Contributor Author

I've fleshed out the proper implementation for the functions/mixins. A couple of notable things:

  1. Option getters get an extra utilities-get-values($utility-name) function. It returns the values for the utility, always as a map (hiding the fact that the values could be lists or even strings). I needed it for implementing some of the mixins and figured it could be useful to developers too.
  2. Adding/removing values will turn the values option for that utility into a proper map, if it previously was a list or a string. I'm not sure if it's a big deal or not. Only thing I could think would break out of it is if:
    • someone already uses Bootstrap,
    • has some code reading the values configuration of a utility themselves for whatever reason.
    • have that code expect a list or string for the values of the property
    • start using utilities-add-values/utilities-remove-values once it's out
      Feels like it's something that can be sorted out through documentation.
  3. Implementation is not defensive at all: option getters return null if the any part of the path is missing, set-options creates new utilities if the utility name does not exist already for example. Do you think it'd be preferable to add some warning/errors to guide developers a little more?

It's tested out using this sass-true test suite (kept on a separate branch to not pollute the project awaiting a decision on the testing strategy). so hopefully as airtight as it can be.

@romaricpascal romaricpascal marked this pull request as ready for review March 14, 2022 10:41
@romaricpascal romaricpascal requested a review from a team as a code owner March 14, 2022 10:41
@romaricpascal romaricpascal changed the title [WIP] Add functions and mixins to manipulate the utlities map Add functions and mixins to manipulate the utlities map Mar 14, 2022
@WinterSilence
Copy link
Contributor

owners don't like when function/mixin not used anywhere :(

@mdo
Copy link
Member

mdo commented Mar 14, 2022

Loving this, @romaricpascal!

We'll need some documentation added to the Utilities API page I think. Maybe add a new section under ## Using the API to lead with these new mixins? Pulling some examples from your sass-true demo test suite would be super helpful for showing how each mixin works.

I'm not 100% sure on needing to document each function, but I defer to you on that. Could be nice to have a table maybe for those, but the mixins are my main concern.

@mdo
Copy link
Member

mdo commented Mar 14, 2022

Oh! And I'd love to see the sass-true branch as a PR whenever you're able! I can help add more tests if you want to open a first step PR :).

@romaricpascal romaricpascal force-pushed the utilities-functions-mixins branch from cc983cd to cb913d8 Compare March 16, 2022 13:13
@romaricpascal
Copy link
Contributor Author

Loving this, @romaricpascal!

We'll need some documentation added to the Utilities API page I think. Maybe add a new section under ## Using the API to lead with these new mixins? Pulling some examples from your sass-true demo test suite would be super helpful for showing how each mixin works.

I'm not 100% sure on needing to document each function, but I defer to you on that. Could be nice to have a table maybe for those, but the mixins are my main concern.

I updated the "Using the API" section of the "Utility API" page. I left the existing examples in a "Practical example" section, as they answer the more likely updates people would want to do. Maybe they could be put as a last "cookbook-ish" section rather than nested inside the part describing the mixins?

Also found an example in the Color page that was using the $utilities, it's now much leaner with the mixin 😆

Please let me know of any extra updates (or if there's other examples of use of the utilities API in examples I'd have missed).

Oh! And I'd love to see the sass-true branch as a PR whenever you're able! I can help add more tests if you want to open a first step PR :).

I'll prepare that! Pretty excited of the positive welcome it's receiving 😄

@romaricpascal
Copy link
Contributor Author

romaricpascal commented Mar 16, 2022

The sass-true PR is open there: #36029.

@mdo mdo force-pushed the utilities-functions-mixins branch from 59824ca to 62e7f9f Compare April 2, 2022 03:54
@mdo mdo requested a review from a team as a code owner April 2, 2022 03:54
@romaricpascal romaricpascal force-pushed the utilities-functions-mixins branch from 62e7f9f to 23e5082 Compare September 9, 2022 10:13
@romaricpascal
Copy link
Contributor Author

romaricpascal commented Sep 9, 2022

Whoops, looks like I nuked my branch on my repo. I'll see if I can recover it somehow, sorry. I'm not quite sure what happened there. Think I may have ran git pull main on the branch rather than rebased main 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants