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

Upgrade bootstrap alpha to edx-bootstrap (beta) #31

Merged
merged 1 commit into from
Sep 11, 2017

Conversation

thallada
Copy link
Contributor

@thallada thallada commented Sep 8, 2017

The includePaths addition to sass-loader is because edx-bootstrap doesn't use webpack, and requires bootstrap sass like this:

@import 'bootstrap/scss/functions';

Instead of:

@import '~bootstrap/scss/functions';

(I'll add a note about this to their README.)

The rest is addressing all of the variable changes/removals between bootstrap alpha6 and beta.

@arizzitano I have no idea what the paragon-reset looks like now so you might want to test to see if it still looks right wherever it's being used. I just did my best to get it to compile.

@coveralls
Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage remained the same at 98.396% when pulling 8c5b279 on thallada/edx-bootstrap into 0b291c8 on master.

@@ -1,3 +1,4 @@
@import '~edx-bootstrap/sass/open-edx/theme';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope that this won't break theming in clients of paragon since all of the variables in the theme are defined with !default. Without this here the scss won't compile.

Copy link
Contributor

Choose a reason for hiding this comment

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

😬

Seems like an issue all right. How can we test that/how can we remove that dependency?

Copy link
Contributor

Choose a reason for hiding this comment

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

!default is kind of the opposite of !important. Any variable definition in the same scope without !default will override the default -- even if that variable is defined before the default. So as long as themes define these vars without !default, we should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we had concerns about this, I tested it out to make sure. Say we have a theme:

// ./src/_theme.scss
$red: #d9534f;
$brand-primary: $red;

As long as we import it before @import "paragon-reset";, then that theme will override anything in the openedx theme. The webpack config in our project for doing that:

{
  loader: 'sass-loader',
    options: {
      sourceMap: true,
      data: '@import "red-theme"; @import "paragon-reset";',
      includePaths: [
        path.join(__dirname, '../src'),
        path.join(__dirname, '../node_modules/paragon/src/utils'),
        path.join(__dirname, '../node_modules'),
      ],
   },
},

Copy link
Contributor

@efischer19 efischer19 left a comment

Choose a reason for hiding this comment

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

Everything seems logical to me; that theming issue might be a bear though. Let's try to get that straightened out.

@@ -1,3 +1,4 @@
@import '~edx-bootstrap/sass/open-edx/theme';
Copy link
Contributor

Choose a reason for hiding this comment

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

😬

Seems like an issue all right. How can we test that/how can we remove that dependency?

Copy link
Contributor

@efischer19 efischer19 left a comment

Choose a reason for hiding this comment

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

(I don't feel like my review should block merging though - if you and @arizzitano come to a consensus, run with it)

Copy link
Contributor

@arizzitano arizzitano left a comment

Choose a reason for hiding this comment

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

👍 thanks for this @thallada!

@@ -1,3 +1,4 @@
@import '~edx-bootstrap/sass/open-edx/theme';
Copy link
Contributor

Choose a reason for hiding this comment

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

!default is kind of the opposite of !important. Any variable definition in the same scope without !default will override the default -- even if that variable is defined before the default. So as long as themes define these vars without !default, we should be fine.

@arizzitano
Copy link
Contributor

Just noticed some issues with the dropdown styling -- I can either fix them in this PR or do a followon PR, whatever's easier for you.

@thallada thallada merged commit 6044f81 into master Sep 11, 2017
@thallada thallada deleted the thallada/edx-bootstrap branch September 11, 2017 15:08
PKulkoRaccoonGang pushed a commit to raccoongang/paragon that referenced this pull request Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants