Skip to content

Conversation

@pat270
Copy link
Contributor

@pat270 pat270 commented Nov 7, 2017

Changing $component-active-bg and $component-active-color only changes the style on some components, it looks like custom checkbox, custom radio, and pagination should also use these variables.

The second commit bases focus colors for Form elements and Buttons on $component-active-bg. This seemed logical to me, but wasn't sure if this behavior fits with Bootstrap 4's roadmap.

@XhmikosR XhmikosR requested a review from mdo November 8, 2017 00:22
@andresgalante
Copy link
Collaborator

Hey @pat270 😄 I think we should add them in places that have a direct relationship, like pagination. But I wouldn't add them to variables that don't have a tie to the component- variable.

To me, yes to you first commit, no to the second one. But I'd definitely add new component variables to standardise the way we treat components.

Thanks a lot!

@pat270
Copy link
Contributor Author

pat270 commented Nov 9, 2017

@andresgalante, An idea that I had that I didn't mention is doing away the $component-active-* variables (a breaking change that you might not want to introduce in beta). One size fits all global color variables are hard to implement because of the number of components and colors per component. I like the idea of Bootstrap's color system minimizing the number of global colors and taking it on a per component basis.

@andresgalante
Copy link
Collaborator

andresgalante commented Nov 9, 2017

I am for any kind of .component-* variable to standardise components (I brought one up here #23674). So I'd say yes, but since it adds a layer of abstraction I'd wait for @mdo to comment.

@tmorehouse
Copy link
Contributor

tmorehouse commented Nov 20, 2017

Would this include a fix for focus styling for the active link in pagination? Currently for keyboard users it is not possible to tell if the active page link is focused. I'm currently using the following CSS to add the focus styling:

.pagination > .page-item > .page-link.active:focus {
    box-shadow: 0 0 0 3px rgba(0,123,255,.5);
    z-index: 2;
}

image

@pat270
Copy link
Contributor Author

pat270 commented Nov 20, 2017

@tmorehouse This just lets you change the background-color and color of .page-link.active through the $component-active-bg and $component-active-color variables.

For the focus style, it looks like Bootstrap just uses the browser default focus outline if .page-link is an <a> tag. Seems like the only way to customize it is to do what you did.

@tmorehouse
Copy link
Contributor

@pat270 ok, cool.

I've opened a new issue #24838 regarding the current unnoticeable focus styling

@mdo
Copy link
Member

mdo commented Dec 28, 2017

Updated to resolve conflicts in _variables.scss from consolidating input/btn styles. Should be good to go, but if you could re-review, that'd be awesome!

@pat270
Copy link
Contributor Author

pat270 commented Jan 2, 2018

This LGTM.

@mdo mdo mentioned this pull request Jan 12, 2018
@XhmikosR XhmikosR merged commit 17b0472 into twbs:v4-dev Jan 13, 2018
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.

5 participants