-
Notifications
You must be signed in to change notification settings - Fork 44
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
[TASK] use bootstrap variables in custom LESS files #294
Conversation
b10312e
to
dbac105
Compare
Resources/Public/less/bootstrap.less
Outdated
@carousel-indicator-bg: #fff; | ||
|
||
// Logo Carousel | ||
@logo-carousel-control-color: #999; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, I would like to decrease the number of unique color variables, even if we will need to simplify design for it. So, I am not sure about this part, might be we can skip some specific variables for content elements (sl-overlay-background, quote-footer-title-color, logo-carousel-control-color
)? I mean add one/two more general color variables, and use it inside on specific content element styling file but not in customVariables
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sure. I like to decrease the number of different styles for same behaviour. E. q. same color for controls in all sliders. But if we use one variable for control color i would leave it in customVariables
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I understand you, but not agree. I still think that we should build our elements using general variables, and if you find some content element with a specific color, that's mean we need to fix this color to use some existed variable instead of creating new variables, even if we will need to change the design for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But as I told, I understand you, so I suggest to create variables for specific content element, but with default general color, which you can change later on with custom colors. For example:
@quote-footer-title-color: @main-text-color;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to remove this 'issues' which we discussing and create new PR with it and continue discussing it there, then I will merge this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! But what exactly should I do now for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Remove this files from commit:
Resources/Public/less/bootstrap.less
Resources/Public/less/main.less
- Fix conflicts
- Fix stylelint warnings in typicalContentElements.less
- Modify custom variables:
- from:
@header-top-search-border-color: #eee;
// Bootstrap Carousel
@carousel-indicator-bg: #fff;
// Logo Carousel
@logo-carousel-control-color: #999;
// Quote
@quote-footer-title-color: #646464;
// Simple Lightbox
@sl-overlay-background: #333;
@sl-navigation-color: #fff;
- to:
@header-top-search-border-color: @border-color;
// Bootstrap Carousel
@carousel-indicator-bg: @main-body-bg;
// Logo Carousel
@logo-carousel-control-color: lighten(@main-text-color, 10%);
// Quote
@quote-footer-title-color: @main-text-color;
// Simple Lightbox
@sl-overlay-background: darken(@main-text-color, 30%);
@sl-navigation-color: @main-body-bg;
Resources/Public/less/bootstrap.less
Outdated
@@ -868,6 +868,11 @@ | |||
//** Horizontal line color. | |||
@hr-border: @gray-lighter; | |||
|
|||
// Custom media queries breakpoints | |||
@screen-xs--custom: 520px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why such numbers, just curious? (520,1090,1300)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were all already there. I only replaced it by variables :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, my mistake :) But still, then better to change that css to use regular Bootstarap media queries to keep a number of variables small as much as possible.
dbac105
to
b53ebf7
Compare
@dmh Hey dmytro, I updated the commit. Please check |
No description provided.