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

V4 variable reuse #23748

Closed
wants to merge 11 commits into from
Closed

V4 variable reuse #23748

wants to merge 11 commits into from

Conversation

joneff
Copy link
Contributor

@joneff joneff commented Aug 30, 2017

In a nutshell:

  • extract color-yiq mixin to a generic contrast-yiq function to be used for ensuring contrast;
  • introduce $component-* variables linked to $body- variables;
  • link the following to $body- vars:
    • input; rationale: usually have the same background as body;
    • table; rationale: transparent background; usually sit against body;
  • link the following to $component- vars:
  • card, dropdowns, list groups, modals and popovers; rationale: one might want to change all components at once
  • ensured contrast:
    • in input, tooltip and progressbar, between text and background
    • in table, for adequate hover / active background (with opacity)
    • in the rest of the components, for adequate border color (with opacity)

Few notes:

  • with the current variable set, the output will not be changed. It's only when one starts to play around;
  • while modal and cards have a dedicated color for background, they lack one for color. In terms, it's inherited from the nearest ancestor;

Related to #23595

@mdo
Copy link
Member

mdo commented Oct 18, 2017

Just a quick note: I've opted to merge #23943, so we'll need to revisit the variable reuse here.

@joneff
Copy link
Contributor Author

joneff commented Oct 18, 2017

Will do, as well as other improvements

@joneff
Copy link
Contributor Author

joneff commented Dec 6, 2017

@mdo, I've rebased the branch to catch up. Here are the notable changes:

  • opacity-to-mix yields a solid color from rgba input by providing backcolor; used to determine a contrast color for $card-cap-bg if it's semi-opaque
  • color-difference, color-brightness and test-contrast functions to deal with determining contrast between two colors. Algorithm is taken from https://www.w3.org/TR/AERT/#color-contrast. (As a note, even though I use rgb to yiq, in the spec the value is called "color brightness", hence the alias method.)
  • use-if-contrast will use the provided color if the contrast is sufficient; if not it will fallback to contrast-yiq functions. Note, I am willing to change the name but to what?
  • speaking of it, I've intentionally left it in, since my version provides two extra arguments allowing it to be customized on a per case basis, where as the current one is controlled on a global level.

The rest of the changes use the aforementioned functions to ensure contrast between a components background and it's color. All in all, colors propagate roughly in the following sequence body -> components -> specific components.

@XhmikosR
Copy link
Member

XhmikosR commented Nov 4, 2018

@mdo: thoughts?

@joneff joneff requested a review from a team as a code owner November 7, 2018 09:40
@MartijnCuppens
Copy link
Member

MartijnCuppens commented Jan 5, 2019

This PR would indeed give you more control over the color of some components, but I personally also like the inheritance of .text-* utility classes set in the components parent. Also this introduces a lot of new functions which aren't documented and make the the variables file harder to read for those who are trying to figure out how Bootstrap works.

@mdo, @andresgalante what do you think?

@joneff
Copy link
Contributor Author

joneff commented Jan 6, 2019

I would, of course, document the hell out of everything, as long as I know I am on the right path. I am also open to variable name change; function renaming; squashing ... you name it.

That is, if someone actually gave feedback ;)

@MartijnCuppens
Copy link
Member

Sure, good call to not to document everything in this phase. I also think what you want to achieve is something valuable, and people can still set variables to inherit if they want to inherit colors from their parent components like we have now. This would indeed be an excellent feature for those who are familiar with the whole variables file.

My major concern is that this will overcomplicate things too much for those who are pretty new to sass for example. They 'll need to learn all the functions before they can tweak all these things.

@MartijnCuppens
Copy link
Member

We've fixed the ability to configure the component colors with #28035 in the meantime. This covers the ability to change colors per component.

Changing the component color based using these functions feels a bit over engineered imo. We'll pass on this.

@joneff joneff deleted the v4-variable-reuse branch February 21, 2019 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants