Problem/Motivation

Starterkit sass styles don't pass sass-lint check

components/base/forms/_forms.scss
  36:1  error  Please check validity of the block starting from line #36  Fatal

✖ 1 problem (1 error, 0 warnings)

This error on line with IE6,7 hacks http://cgit.drupalcode.org/zen/tree/STARTERKIT/components/base/forms/_fo... 

    // Improve appearance and consistency with IE 6/7.
    *vertical-align: middle;

the same for lines 183, 184
http://cgit.drupalcode.org/zen/tree/STARTERKIT/components/base/forms/_fo...

Steps to reprodule

  • drush dl zen-8.x-7.x && cd zen
  • npm install
  • gulp lint:sass

Proposed resolution

Based on https://www.drupal.org/node/1569578
Let's remove styles for add support of IE6,7

CommentFileSizeAuthor
#9 interdiff-2827085-5-9.txt3.89 KBHOG
#9 fatal_sass_lint_error-2827085-9.patch3.74 KBHOG
#5 fatal_sass_lint_error-2827085-5.patch2.22 KBfinnsky
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andriyun created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

I think we should remove this because d8 does not support ie 6-8 https://www.drupal.org/node/1569578

andriyun’s picture

andriyun CreditAttribution: andriyun at Skilld, Drupal Ukraine Community commented
Issue summary: View changes
Issue tags: +code cleanup
andriyun’s picture

andriyun CreditAttribution: andriyun at Skilld, Drupal Ukraine Community commented
Issue summary: View changes
finnsky’s picture

finnsky CreditAttribution: finnsky at Skilld commented
FileSize
fatal_sass_lint_error-2827085-5.patch2.22 KB

Hi!
Cleaned IE7 specific styles. Not all of them gave error, but when you start to kick IE, it's not easy to stop:)
In case of that issue this seems reasonable for me, because theme (and subtheme) can support IE6-8 itself, but it has no reasons if core css/js will not work there.
Please review. Thanks

finnsky’s picture

finnsky CreditAttribution: finnsky at Skilld commented
Status: Active » Needs review
andriyun’s picture

andriyun CreditAttribution: andriyun at Skilld, Drupal Ukraine Community commented

Ok for me +1 to RTBC

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

Let's see what maintainer thinks, I see no reason to support ie6-8

HOG’s picture

HOG CreditAttribution: HOG at Skilld, Drupal Ukraine Community commented
Status: Reviewed & tested by the community » Needs review
FileSize
fatal_sass_lint_error-2827085-9.patch3.74 KB
interdiff-2827085-5-9.txt3.89 KB

I also fixed some sass lint warnings.

Christian DeLoach’s picture

Christian DeLoach CreditAttribution: Christian DeLoach commented

I think the changes should stay focused and be limited to the original issue's objective. Adding the additional SASS lint warning fixes have already been covered by other issues (e.g. https://www.drupal.org/node/2751687) and should be discussed and patched there. Someone looking to patch the forms.scss may not wish to apply the changes to eliminate the "Pseudo-elements must start with double colons" warnings. While I appreciate the contribution, I recommend apply the patch from #5 which addresses the issue reported by the original poster.