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/helpers split & utilities API #28445

Merged
merged 1 commit into from May 23, 2019

Conversation

MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Mar 10, 2019

Fixes #28367

Split up helpers/utilities and utilities API for extending or getting more control over the generated utilities.

Features

  • Split between helpers & utilities
  • Utility API which allows changing & extending utility classes
  • Documentation about the API
  • Documentation split between helpers & utilities.
  • New map-get-multiple() function to get multiple keys from a map

Notes

  • I left some utilities because their naming required to create a utility group per class, which beats the purpose of the API. Not sure what to do with them, but for me, it fine leaving them in the utility folder for now.
  • I had to increase the bundle sizes because the vertical & horizontal spacing utilities are generated differently

New documentation about the utility API:
https://deploy-preview-28445--twbs-bootstrap.netlify.com/docs/4.3/utilities/api/

@MartijnCuppens MartijnCuppens added this to Inbox in v5 via automation Mar 10, 2019
scss/utilities/_utilities.scss Outdated Show resolved Hide resolved
scss/utilities/_utilities.scss Outdated Show resolved Hide resolved
scss/bootstrap-grid.scss Outdated Show resolved Hide resolved
@mdo
Copy link
Member

mdo commented Mar 10, 2019

For viewport utilities, I'm thinking .vw-100 and .vh-100 for their classes as the names are implicit to their unit of measurement and their intended use (100vw and viewport width 100%).

@mdo
Copy link
Member

mdo commented Mar 11, 2019

So here's a baseline version of what I'm thinking about—one giant Sass map for all $utilities and nested @each loops for generating them. No additional map of map-gets. I've punted on all the more complex stuff though (any spacing utils, anything with a map-merge, etc) though for demo purposes. Will need help with that if we go this route.

https://www.sassmeister.com/gist/efa2593de91ddcc60808c44fe1ea9301

Thoughts?

@MartijnCuppens
Copy link
Member Author

I guess you're talking about the map-gets in the scss/bootstrap-grid.scss? I did that to prevent the repetition of properties here, but maybe I should tackle this in a function where you can send the keys of the utilities you need to?

Or am I missing something here?

@mdo
Copy link
Member

mdo commented Mar 11, 2019

I guess you're talking about the map-gets in the scss/bootstrap-grid.scss?

Oh! Damn, yes I am! I was completely off base, apologies!

@XhmikosR
Copy link
Member

@MartijnCuppens: something isn't right with this branch. You are removing files you are not supposed to touch here.

@MartijnCuppens MartijnCuppens force-pushed the master-mc-utilities-helpers-split branch from b1a7b1a to e9054f6 Compare March 14, 2019 12:40
@MartijnCuppens
Copy link
Member Author

@XhmikosR, fixed it. @mdo, I simplified the bootstrap-grid.scss with a new map-get-multiple function which prevents repetition of the map-get function.

This is still a work in progress, I'm personally going to give this PR more priority to avoid the situations like th Hugo branch where we needed to rebase it every so often.

@MartijnCuppens MartijnCuppens mentioned this pull request Mar 14, 2019
@mdo
Copy link
Member

mdo commented Mar 15, 2019

@MartijnCuppens Thoughts on ditching the map-merge aspect of this? Had some thoughts on this in another thread where if memory serves, we can perhaps ditch it? #27927 (comment)

@MartijnCuppens
Copy link
Member Author

I've opened #28506 to track this, that's a decision we can make independent from this, but I think it's a good idea. This will make it possible to ditsh this part here and simplify the map-gets you mentioned before:

// Utilities can also be removed, for example if you want to remove the `float` utilities:
// $utilities: (
// "float" :false,
// );

@MartijnCuppens MartijnCuppens force-pushed the master-mc-utilities-helpers-split branch from 11db464 to c5a0434 Compare March 17, 2019 10:44
@MartijnCuppens MartijnCuppens force-pushed the master-mc-utilities-helpers-split branch from c5a0434 to 09ede1e Compare April 6, 2019 19:36
@MartijnCuppens MartijnCuppens force-pushed the master-mc-utilities-helpers-split branch 2 times, most recently from 3b8d66d to 25918f6 Compare April 21, 2019 12:24
@MartijnCuppens MartijnCuppens changed the title Utilities/helpers split & utilities "API" Utilities/helpers split & utilities API Apr 21, 2019
@MartijnCuppens MartijnCuppens marked this pull request as ready for review April 21, 2019 12:27
@MartijnCuppens MartijnCuppens requested a review from a team as a code owner April 21, 2019 12:27
@MartijnCuppens MartijnCuppens force-pushed the master-mc-utilities-helpers-split branch from 25918f6 to 840526b Compare April 24, 2019 15:54
@MartijnCuppens MartijnCuppens moved this from Inbox to Review in v5 Apr 24, 2019
@MartijnCuppens MartijnCuppens force-pushed the master-mc-utilities-helpers-split branch from 840526b to fff0f2d Compare April 27, 2019 14:17
@MartijnCuppens MartijnCuppens force-pushed the master-mc-utilities-helpers-split branch from fff0f2d to d7c0271 Compare May 7, 2019 16:49
@MartijnCuppens MartijnCuppens force-pushed the master-mc-utilities-helpers-split branch from d7c0271 to 9dd51ee Compare May 19, 2019 16:43
@MartijnCuppens
Copy link
Member Author

Ping @twbs/css-review for review

@MartijnCuppens MartijnCuppens force-pushed the master-mc-utilities-helpers-split branch from 9dd51ee to 82f6f29 Compare May 23, 2019 06:47
@MartijnCuppens
Copy link
Member Author

FYI: the diff between classes: https://www.diffchecker.com/noUio3fe. Only some responsive .text-justify classes were added.

Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

Let's do this!

v5 automation moved this from Review to Approved May 23, 2019
@MartijnCuppens MartijnCuppens merged commit 769c8d8 into master May 23, 2019
v5 automation moved this from Approved to Shipped May 23, 2019
@MartijnCuppens MartijnCuppens deleted the master-mc-utilities-helpers-split branch May 23, 2019 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5
  
Shipped
Development

Successfully merging this pull request may close these issues.

Split utilities from property utilties and get more control over generation of property utilities
3 participants