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 check require fields for Utilities API #34529

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

Conversation

Psixodelik
Copy link
Contributor

@Psixodelik Psixodelik commented Jul 19, 2021

closes #34523

Hi

I added required fields validation when generating utilities using the Utilities API. For this, I added a new mixin to _functions.scss (avoiding code duplication).

The fields for checking are taken from the $required-fields variable (maybe it should be taken out in _variables.scss)

@Psixodelik Psixodelik requested a review from a team as a code owner July 19, 2021 12:48
scss/_functions.scss Outdated Show resolved Hide resolved
scss/_functions.scss Outdated Show resolved Hide resolved
@@ -1,6 +1,20 @@
// Utility generator
// Used to generate utilities & print utilities
@mixin generate-utility($utility, $infix, $is-rfs-media-query: false) {
@mixin generate-utility($utility, $utility-name, $infix, $is-rfs-media-query: false) {
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this new argument (which would lead to a breaking change) since we can get the field using $properties: map-get($utility, property) and nth($properties, 1) as it's done L20, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that we are already passing the values, but not the name of the utility. nth($properties, 1) can return multiple values or nothing at all if the property field does not exist.

I can move the name of the utility to the end of the argument chain and make it an optional field. Then I'll add an extra check for the existence of this argument. This will not break the ready-made code for developers.

And you can apply by name. For example, like this: @include generate-utility ($utility, $infix, $utility-name: $key);

scss/mixins/_utilities.scss Outdated Show resolved Hide resolved
scss/mixins/_utilities.scss Show resolved Hide resolved
scss/mixins/_utilities.scss Show resolved Hide resolved
@@ -10,7 +10,7 @@
// The utility can be disabled with `false`, thus check if the utility is a map first
// Only proceed if responsive media queries are enabled or if it's the base media query
@if type-of($utility) == "map" and (map-get($utility, responsive) or $infix == "") {
@include generate-utility($utility, $infix);
@include generate-utility($utility, $key, $infix);
Copy link
Member

Choose a reason for hiding this comment

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

This change should really be avoided if possible, code relying on generate-utility() outside of our core will break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it has something to do with this comment: #34529 (comment)

This part needs to be redone. Look at the solution in the comments, please

@Psixodelik
Copy link
Contributor Author

@ffoodd Hi

I rewrote the code. I had to leave the name of the utility as an argument, but I made this argument optional, and you can pass it as a named argument. That is, the call can be like this:

@include generate-utility($utility, $infix, $utility-name: $ key);

or

@include generate-utility ($utility, $infix, true);

or

@include generate-utility ($utility, $infix, true, $utility-name: $ key);

etc. This will prevent other developers from breaking code. I haven't come up with any other beautiful solution :( Changing the $ utility array will do more harm than good.

Please see PR. Maybe we can tweak something else. I suggest leaving the checks themselves at the beginning of the mixin so as not to scatter conditions throughout the code. Thus, we will leave the check inside one block of code.

@Psixodelik
Copy link
Contributor Author

@ffoodd Hi, Are there edits on this PR?

@mdo mdo added this to In progress in v5.3.0 via automation Apr 12, 2022
@mdo
Copy link
Member

mdo commented Apr 12, 2022

I'll be swinging around to this for our v5.3.0 release—sorry for the delay!

@mdo
Copy link
Member

mdo commented Dec 28, 2022

Since this PR was opened, we've expanded the utility API to include CSS variable based utilities, which don't have a property parameter. So the only required field would be values. We could perhaps revisit this and split it based on what's a normal utility vs CSS var utility.

But, after a main merge here, it does work as expected! Super awesome to see.

Warning: Missing 'property' for 'border-width' utility
    scss/_functions.scss 306:5          -validate-utilities-field()
    scss/mixins/_utilities.scss 12:7    generate-utility()
    scss/utilities/_api.scss 13:9       @content
    scss/mixins/_breakpoints.scss 68:5  media-breakpoint-up()
    scss/utilities/_api.scss 5:3        @import
    scss/bootstrap.scss 51:9            root stylesheet

Warning: Missing 'property' for 'border-opacity' utility
    scss/_functions.scss 306:5          -validate-utilities-field()
    scss/mixins/_utilities.scss 12:7    generate-utility()
    scss/utilities/_api.scss 13:9       @content
    scss/mixins/_breakpoints.scss 68:5  media-breakpoint-up()
    scss/utilities/_api.scss 5:3        @import
    scss/bootstrap.scss 51:9            root stylesheet

Warning: Missing 'property' for 'text-opacity' utility
    scss/_functions.scss 306:5          -validate-utilities-field()
    scss/mixins/_utilities.scss 12:7    generate-utility()
    scss/utilities/_api.scss 13:9       @content
    scss/mixins/_breakpoints.scss 68:5  media-breakpoint-up()
    scss/utilities/_api.scss 5:3        @import
    scss/bootstrap.scss 51:9            root stylesheet

Warning: Missing 'property' for 'bg-opacity' utility
    scss/_functions.scss 306:5          -validate-utilities-field()
    scss/mixins/_utilities.scss 12:7    generate-utility()
    scss/utilities/_api.scss 13:9       @content
    scss/mixins/_breakpoints.scss 68:5  media-breakpoint-up()
    scss/utilities/_api.scss 5:3        @import
    scss/bootstrap.scss 51:9            root stylesheet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.3.0
In progress
Status: Needs review
Development

Successfully merging this pull request may close these issues.

Utilities API. Checking for the existence of required fields
3 participants