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.

Sign up for GitHub

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

Jump to bottom

Scss bootsrap 4 #4099

Merged
merged 5 commits into from Aug 7, 2018
Merged

Scss bootsrap 4 #4099

merged 5 commits into from Aug 7, 2018

Conversation

ger-benjamin
Copy link
Member

@ger-benjamin ger-benjamin commented Aug 6, 2018 •
edited

@ger-benjamin ger-benjamin added this to the 2.4 milestone Aug 6, 2018
sbrunner
sbrunner approved these changes Aug 7, 2018
View reviewed changes
contribs/gmf/src/sass/vars_only.scss Outdated
$border-radius-base: 2px !default;
$border-radius-small: 1px !default;
$border-radius-base: 0 !default;
$border-radius-small: 0 !default;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we remove them and use $border-radius-sm and $border-radius

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, I prefer to use our own variable ($border-radius-base) So If customers want a radius with bootstrap thing they can have it. And using our own variable, we are not dependant of boostrap as well.

We never use $border-radius-small, I remove it.

Copy link
Member

Choose a reason for hiding this comment

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

OK :-)

contribs/gmf/src/sass/vars_only.scss Outdated
$input-border-focus: darken($brand-primary, $standard-variation) !default;

$fa-font-path: "~font-awesome/fonts";

// Bootstrap 3 compatibility layer
$border-radius: $border-radius-base !default;
$border-radius-lg: $border-radius-base !default;
$border-radius-sm: $border-radius-base !default;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be $border-radius-base / 2 !default?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can, but we always have border-radius = 0 in our code.

Note also that in bootstrap values are (hardcoded at each level):

$border-radius:               .25rem !default;                                                                     
$border-radius-lg:            .3rem !default;                                                                      
$border-radius-sm:            .2rem !default;

Copy link
Member

Choose a reason for hiding this comment

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

OK :-)

contribs/gmf/src/controllers/desktop.scss
@@ -247,7 +246,7 @@ gmf-backgroundlayerselector {
$theme-selector-columns: 2;
.gmf-theme-selector li {
float: left;
width: calc((100% - $theme-selector-columns * 2 * $half-app-margin) / $theme-selector-columns);
width: calc((100% - #{$theme-selector-columns} * 2 * #{$half-app-margin}) / #{$theme-selector-columns});
Copy link
Member

Choose a reason for hiding this comment

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

What's this syntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

The equivalent to calc(100% - ~"@{theme-selector-columns} * 2 ...") in less

So a manner to use calc with sass variables

Copy link
Member

Choose a reason for hiding this comment

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

Merci :-)

@ger-benjamin ger-benjamin force-pushed the scss_bootsrap_4 branch 2 times, most recently from 9ba1976 to a2f5146 Compare August 7, 2018 08:27
sbrunner
sbrunner approved these changes Aug 7, 2018
View reviewed changes
contribs/gmf/src/controllers/desktop.scss
@@ -247,7 +246,7 @@ gmf-backgroundlayerselector {
$theme-selector-columns: 2;
.gmf-theme-selector li {
float: left;
width: calc((100% - $theme-selector-columns * 2 * $half-app-margin) / $theme-selector-columns);
width: calc((100% - #{$theme-selector-columns} * 2 * #{$half-app-margin}) / #{$theme-selector-columns});
Copy link
Member

Choose a reason for hiding this comment

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

Merci :-)

contribs/gmf/src/sass/vars_only.scss
$border-radius-sm: $border-radius-base !default;
$font-size-base: 0.8rem !default;
$input-btn-padding-y: 0.25rem !default;
$input-btn-padding-x: 0.5rem !default;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we set the comment
# Bootstrap 4 customization
for this block?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean you want that I change the // Bootstrap 3 compatibility layer comment to yours ?

@ger-benjamin ger-benjamin force-pushed the scss_bootsrap_4 branch 2 times, most recently from b417464 to e7fe0e0 Compare August 7, 2018 10:55
@ger-benjamin ger-benjamin changed the title WIP Scss bootsrap 4 Scss bootsrap 4 Aug 7, 2018
@ger-benjamin
Copy link
Member Author

Still a lot to do (mainly in the right panels) but I want to merge
It's already far better now :-)

@ger-benjamin ger-benjamin merged commit 78c4ceb into master Aug 7, 2018
@ger-benjamin ger-benjamin deleted the scss_bootsrap_4 branch August 7, 2018 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbrunner sbrunner sbrunner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.4
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ger-benjamin @sbrunner