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

v4: Fix Dart Sass divide usage #34386

Closed
wants to merge 6 commits into from
Closed

v4: Fix Dart Sass divide usage #34386

wants to merge 6 commits into from

Conversation

slavede
Copy link

@slavede slavede commented Jun 30, 2021

Applied sass-migrator for division deprecation issue. Fixes #34353

@slavede slavede requested a review from a team as a code owner June 30, 2021 10:13
@slavede slavede mentioned this pull request Jun 30, 2021
Closed
@slavede slavede mentioned this pull request Jun 30, 2021
Closed
tu4mo
tu4mo reviewed Jun 30, 2021
View reviewed changes
scss/vendor/_rfs.scss Outdated Show resolved Hide resolved
DerZyklop
DerZyklop reviewed Jul 1, 2021
View reviewed changes
Copy link

@DerZyklop DerZyklop left a comment

Choose a reason for hiding this comment

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

@slavede changed every / 2 to * 0.5. It would be a no-brainer to at least merge these changes into existing bootstrap v4

@XhmikosR XhmikosR changed the title Applied sass-migrator for division deprecation issue. Fixes #34353 v4: Fix Dart Sass divide usage Jul 1, 2021
@simonihmig simonihmig mentioned this pull request Jul 4, 2021
Closed
ffoodd
ffoodd requested changes Jul 6, 2021
View reviewed changes
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

RFS should be updated in a separate PR I guess, or at least make sure you're using latest RFS instead of modifying it.

Minor a few unrelated line breaks, this looks great: thanks a lot for your effort ❤️

scss/mixins/_forms.scss
@@ -10,6 +10,7 @@
//
// Example usage: change the default blue border and shadow to white for better
// contrast against a dark gray background.

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change

scss/utilities/_embed.scss
@@ -1,5 +1,6 @@
// Credit: Nicolas Gallagher and SUIT CSS.


Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change

scss/vendor/_rfs.scss
@@ -9,6 +9,7 @@
// Configuration
Copy link
Member

Choose a reason for hiding this comment

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

RFS is a vendor, please do not edit it directly. It needs to be updated from upstream, see RFS repository.

Copy link
Author

Choose a reason for hiding this comment

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

But it doesn't say inside which exact version of rfs we have here? Should I update the latest? If so, it will pull in a lot more changes to it...this way, it would just fix the warnings...and leave functionality/behavior the same

Copy link
Member

Choose a reason for hiding this comment

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

Leave RFS for now, we'll upgrade it in another PR. @XhmikosR Does this sound right to you?

@shauke shauke mentioned this pull request Jul 6, 2021
Closed
This was referenced Jul 7, 2021
Closed
Closed
@XhmikosR
Copy link
Member

@ffoodd Yeah, RFS shouldn't be touched for sure.

Generally, we should be very careful with this PR. It still has some issues on main, I wouldn't hurry backporting it unless we are 100% sure it's OK.

@XhmikosR XhmikosR requested a review from mdo July 19, 2021 15:05
@mdo mdo mentioned this pull request Jul 25, 2021
Merged
1 task
@mdo
Copy link
Member

mdo commented Jul 25, 2021

Fixed linting issues, removed unrelated changes, and removed RFS in #34571.

@mdo mdo closed this Jul 25, 2021
@36degrees 36degrees mentioned this pull request Oct 5, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tu4mo tu4mo tu4mo left review comments

@ffoodd ffoodd ffoodd requested changes

@DerZyklop DerZyklop DerZyklop left review comments

@snewer snewer snewer approved these changes

@mdo mdo Awaiting requested review from mdo

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

Successfully merging this pull request may close these issues.
7 participants
@slavede @XhmikosR @mdo @DerZyklop @ffoodd @snewer @tu4mo