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

style: fix sasslint warnings #11227

Merged
merged 1 commit into from May 2, 2018
Merged

style: fix sasslint warnings #11227

merged 1 commit into from May 2, 2018

Conversation

DanielRuf
Copy link
Contributor

Description

This fixes many of the current sasslint warnings.

Motivation and Context

Currently sasslint warns about many things which are not as they should be according to the sasslint config.

Types of changes

  • Documentation
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing
    functionality to change)

Checklist (all required):

  • I have read and follow the CONTRIBUTING document.
  • There are no other pull request similar to this one.
  • The pull request title is descriptive.
  • The template is fully and correctly filled.
  • The pull request targets the right branch (develop or support/*).
  • My commits are correctly titled and contain all relevant information.
  • My code follows the code style of this project.
  • I have updated the documentation accordingly to my changes (if relevant).
  • I have added tests to cover my changes (if relevant).
  • All new and existing tests passed.

@DanielRuf DanielRuf added the scss label May 1, 2018
@DanielRuf DanielRuf requested a review from ncoden May 1, 2018 21:28
Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

Some new sasslint rules would be useful there. Like sasstools/sass-lint#950. Almost half of the changes in this PR are due to force-element-nesting.

I would prefer to disable this option than to add so much useless nesting.

margin-top: -1 * ($accordionmenu-arrow-size / 2);
#{$global-right}: 1rem;
.is-accordion-submenu-parent {
&:not(.has-submenu-toggle) > a {
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be needed because of the force-element-nesting rule. I find this rule too restrictive and think that we should allow complex selectors when they cannot be marged.

See sasstools/sass-lint#950

But sass-lint seems badly maintained now. 😞


width: $accordionmenu-submenu-toggle-width;
height: $accordionmenu-submenu-toggle-height;

cursor: pointer;
Copy link
Contributor

Choose a reason for hiding this comment

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

cursor should be after border

font-size: map-get($button-sizes, default);
font-weight: $button-font-weight;
-webkit-appearance: none; // sass-lint:disable-line no-vendor-prefixes
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need -webkit-appearance instead of appearance here ? Autoprefixer supports it now.

// No base style is required for solid buttons, do nothing
}
@else if $fill == hollow {
@if $fill == hollow {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could still keep a comment about solid buttons above this.

@DanielRuf
Copy link
Contributor Author

Hm, should we migrate to stylelint and use their scss plugin pack?

@ncoden
Copy link
Contributor

ncoden commented May 1, 2018

stylelint looks great but for now we could simply disable force-*-nesting

@@ -51,7 +51,7 @@ $accordionmenu-submenu-toggle-height: $accordionmenu-submenu-toggle-width !defau
$accordionmenu-arrow-size: 6px !default;

@mixin zf-accordion-menu-left-right-arrows {
.is-accordion-submenu-parent:not(.has-submenu-toggle) > a {
.is-accordion-submenu-parent:not(.has-submenu-toggle) > a { // sass-lint:disable-line force-element-nesting force-pseudo-nesting
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant: disabling it for the entire project in .sass-lint.yml.
You can squash commits after this.

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 meant: disabling it for the entire project in .sass-lint.yml.
You can squash commits after this.

Done

Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

👍 LGTM. Thank you @DanielRuf

@ncoden ncoden merged commit 02d0523 into foundation:develop May 2, 2018
@DanielRuf DanielRuf deleted the style/fix-sasslint-warnings branch May 2, 2018 22:25
ncoden pushed a commit to ncoden/foundation-sites that referenced this pull request Jun 16, 2018
…arnings for v6.5.0

92b2f18 style: fix sasslint warnings

Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
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.

2 participants