Proposed changes luca #2

LucaPipolo · 2017-08-25T09:27:55Z

No description provided.

sun · 2017-08-25T09:33:10Z

assets/scss/bacon.scss

@@ -1,6 +1,6 @@
 //
 // Bacon
-// --------------------------------------------------
+// -----------------------------------------------------------------------------


👎 This is the primary reason for why we should use C-style / CSSDoc syntax for comment blocks instead:

/** * Bacon * * Some optional description here. And you can use most of * the generic *Doc tags/directives, too: * * @param int $font-size * * @see http://www.stack.nl/~dimitri/doxygen/ * @see https://timkadlec.com/2008/12/manageable-css-with-cssdoc/ * @author netzstrategen * @license MIT */

// is the inline comment syntax in all mature programming languages including SCSS; it is wrong to use them for block-level documentation.

Ok but using /* */ the comments will be preserved in the CSS output. No?

However to generate documentation with SassDoc you need to use ///.

No, when configured correctly, only the special syntax /*! for package file headers & legal / license information is retained in the compiled CSS.

To summarize code comment syntaxes:

/*! * Package name header with author and license info. * * Retained in compiled CSS as-is. * * @license MIT */ /** * CSSDoc block-level comment for files, sections, components. * * First summary line is separated with a blank line from description. * Not included in compiled CSS. */ /* var commented = out.code(); */ // Inline comment explaining internal / detailed reasons. // Should be placed on its own line above the commented line. font-size: 100%;

SassDoc uses https://github.com/SassDoc/scss-comment-parser which uses https://github.com/FWeinb/CDocParser, which normally parses C-style comments, but CDocParser was seemingly enhanced specifically for SassDoc to also support the triple-slash /// syntax. 💩

See blockCommentStyle and lineCommentStyle options here: https://github.com/FWeinb/CDocParser#user-content-new-commentextractorcontextparser-opts

Apparently, SassDoc specifically disables block-level comments:
https://github.com/SassDoc/scss-comment-parser/blob/f93499709fd5180950504df4e6f41433c4f91983/index.js#L198-L200

🤦‍♂️

So? Which would be the solution? I never use something different that SassDoc and I didn't find valid alternatives.

Should we:

A. Not create a Sass documentation;
B. Keep using the 💩 /// syntax;

A is a good and interesting question.

We will actually define and document most of our styles in the form of components in the design system, which produces a styleguide on its own.

What's the purpose of SassDoc in relation to that? Which problem are you trying to solve?

Was not really a problem to solve, instead, was more like something to add.

It's true that components structure is pretty clear but SassDoc will document mixins and functions extrapolating Sass comments (unfortunately not C-Styled one) into a page like this one:

I found it extremely useful in some projects I worked with and so I thought could be a cool stuff to introduce.

Maybe should I change idea about? Cons of using it are more than pros?

This is great, thanks Luca. The comment style I used is from Bootstrap which I find to be easy-to-read and have been using for six months or so - there was no real decision-making behind it. I'm aware of Sassdoc (although I've never used it) and it seems to be a good way to go. Automatically generated documentation is a big win. I should get time later in the week to review this properly 👍

sun · 2017-08-25T09:44:29Z

sassdoc/assets/js/main.js

@@ -0,0 +1,56 @@
+/* global document */


Did you intend to commit this package? Why?

True, sorry. We can exclude the whole sassdoc folder since it will be auto generated from gulp.

colourgarden · 2017-08-31T08:58:54Z

gulpfile.js

+        'android 4',
+        'opera 12'
+      ]
+    })


Autoprefixer uses Browserslist which allows you to specify browser support in an external file. They recommend adding the block to package.json so I'll do that. https://github.com/ai/browserslist

Super, agree with you. 👍

colourgarden · 2017-09-01T14:15:29Z

Added a bunch of commits which take in this PR as well as #3 and #6.

Implemented C-style commenting as per CSS Guidelines and Nicolas Gallagher's idiomatic CSS rules. We have two levels (file heading and block comment) plus double-slashes for Sass-specific comments. These are also now documented in index.html.
Added a new Gulp task which uses clean-css (from @fabianmarz PR) plus gulp-rename and gulp-replace to create a minified, production-ready result

@LucaPipolo @fabianmarz Could you take a look at the latest commits and comment please? Let's get all this merged in so we can get started with the component building :)

LucaPipolo · 2017-09-01T14:35:33Z

@colourgarden — looks really good to me, as we discussed during last days on Slack. Great job! 👍

Just one question. I saw you decided to adopt the /// comments to generates the Sassdoc documentation but then removed Sassdoc from gulp. Why? Because of #7 ?

LucaPipolo · 2017-09-01T14:37:24Z

Of course before merging we should clean the tree removing:

Updated Gulp autoprefixer browsers list.
Added property-sort-order-smacss Stylelint plugin.

Instead of reverting them. :)

sun

Awesome job! 👍

sun · 2017-09-01T19:17:18Z

.stylelintrc

@@ -82,6 +83,18 @@
    "number-leading-zero": "always",
    "number-no-trailing-zeros": true,

+    "plugin/selector-bem-pattern": {
+      "componentName": "(([a-z0-9]+(?!-$)-?)+)",


What is this regex trying to match/achieve?

Just the following? [a-z][a-z0-9_-]*[a-z0-9]

If so, let's keep it simple and use something like that instead, because it's also more accurate:

In CSS, identifiers (including element names, classes, and IDs in selectors) can contain only the characters [a-zA-Z0-9] and ISO 10646 characters U+00A0 and higher, plus the hyphen (-) and the underscore (_); they cannot start with a digit, two hyphens, or a hyphen followed by a digit.

https://www.w3.org/TR/CSS21/syndata.html#characters

@LucaPipolo added this.

https://github.com/postcss/postcss-bem-linter#user-content-componentselectors

Describes all valid selector sequences for the stylesheet

So componentName only matches the "block" part of a class, and only these that we consider valid.

This would be a matching regex: \.(?:(?!--)[a-z0-9-])+

But according to https://github.com/davidtheclark/stylelint-selector-bem-pattern#user-content-usage, it seems like componentName just needs to specify the allowed characters for the block element, in our case simply: [a-z0-9-]+

sun · 2017-09-01T19:24:15Z

assets/scss/bacon.scss

-// --------------------------------------------------
+@charset "utf-8";
+
+/*! #copyright DO NOT REMOVE# */


What's going on here? Some placeholder magic, I assume?

Yep, in the gulpfile I added a task (sass-prod) to compile production-ready CSS. It replaces this line with a copyright message which sources its information from package.json so we don't need to remember to update the version number in the Sass files every time.

There are gulp packages where you can just add a message to the start of the compiled file (so avoiding the need for this placeholder line) but the @charset declaration must be the first thing in the file. Because there are some non-alphanumeric characters included (embedded SVG) then Sass will always automatically add a @charset declaration.

sun · 2017-09-01T19:30:02Z

assets/scss/base/_document.scss

-// --------------------------------------------------
+/* ==========================================================================
+   Base page styling
+   ========================================================================== */


Wondering how many of these files actually contain just a single selector… Would it be possible to omit these "banners" altogether?

If necessary, we could group multiple things as files in subdirectories instead.

And we could give each file a meaningful name, such as:

/elements/border-box.scss /elements/table-width.scss

Why do you want to remove them? They won't be included in production CSS and provide clear separators in compiled but expanded CSS generated by the standard scss gulp task. I'd rather enforce a convention of having a banner comment at the top of every file for the few occasions where it is useful (for further explanation) rather than sometimes having one, sometimes not.

Splitting up elements is a good idea - I'll do that.

Elements now split into separate files within base/elements folder.

sun · 2017-09-01T19:33:09Z

assets/scss/base/_document.scss

+ * 2. Set everything to border-box by default but allow per-component changes.
+ *    http:/css-tricks.com/inheriting-box-sizing-probably-slightly-better-best-practice
+ * 3. Ensure the page always fills at least the entire height of the viewport.
+ */


All of these docs —> ❤️❤️❤️ !!!

This is how we should be documenting most of our code, as it helps others to understand what's really going on and make decisions.

sun · 2017-09-01T19:34:02Z

assets/scss/base/_document.scss

+ *    navigating between pages that do/do not have enough content to produce
+ *    scrollbars naturally.
+ * 2. Set everything to border-box by default but allow per-component changes.
+ *    http:/css-tricks.com/inheriting-box-sizing-probably-slightly-better-best-practice


This actually appears to be style affecting all elements, shouldn't it be in elements instead?

It's kinda both but I'll move it into an elements folder as per previous comment.

sun · 2017-09-01T19:38:32Z

assets/scss/base/_elements.scss

+ * 2. Emphasise `alt` text if shown.
+ * 3. Setting `vertical-align` removes the whitespace that appears under `img`.
+ *    elements. Safer alternative to using `display: block;`.
+ * 4. If an image has explicit dimensions set then don't make it responsive.


The CMS will always output dimensions for all images by default (unless we start to make very very granular template variable overrides), so I doubt this rule makes sense?

What about for responsive images where we want them to fill the container (width and height)? Explicit dimensions wouldn't be ideal in those scenarios.

Regular image output produced by the CMS will either use srcset or <picture> (not sure whether the upgrade to picture happened in Drupal core already).

Of course, there might be custom output scenarios in custom templates (such as setting a background-image instead), but we should rather assume that there will be dimensions all images by default.

Ok so should I add height: auto to all images as well then? If we're adding dimensions to all images then they're not going to work properly responsively because the height will stay the same as the width changes = distorted image.

I believe that's what we're doing in all the (WP) projects, yes. I don't know whether that is the best solution.

Here are the current image output templates of Drupal core:
https://github.com/drupal/drupal/blob/8.5.x/core/modules/system/templates/image.html.twig
https://github.com/drupal/drupal/blob/8.5.x/core/modules/responsive_image/templates/responsive-image.html.twig

The best solution would be to remove width and height attributes. But if we can't do this reliably then we should add height: auto as standard to cover for that.

JavaScript libraries for lazy-loading, galleries, and lightboxes usually need the image dimensions before loading an image in order to calculate and prepare the content layout, so I think we can only omit them in limited special cases?

Fixed in fixup commit.

sun · 2017-09-01T19:39:14Z

assets/scss/base/forms/_choice.scss


-[type='checkbox']
+/**
+ * Shared choice element styling


Missing trailing period. (also elsewhere)

Ok will make this a convention by changing throughout and adding to index.html docs.

Corrected throughout. All block-level comments should require a full stop but Sass double-slash comments don't.

In PHP and JS, we require full sentences starting with an uppercase letter and ending in a trailing period for any kind of comment, including inline comments.

The only exceptions to that are @see (because what follows is a URL or function or some other kind of resource identifier) and inline comments that are not natural language (values or references).

Furthermore, inline comments always have to be on their own line, above the commented line of code; i.e.:

$foo = $bar; // wrong // Right. $foo = $bar;

The SCSS footnotes would fall under the exception of references.

colourgarden · 2017-09-06T16:24:02Z

assets/scss/bacon.scss

+  'base/elements/box-sizing',
+  'base/elements/shared',
+  'base/elements/links',
+  'base/elements/images',


@LucaPipolo What was the reason for deleting _elements.scss and including the files individually here?
Forms work in the same way so we should at least be consistent. But I prefer the method of having the additional include file in the folder as it feels neater and should also cut down on dead files.

We investigated base/elements/* as an alternative, but weren't sure whether you intended to have these elements in a particular order.

A separate include file that includes further files is "obscuring" the intended final configuration. I would definitely recommend to keep all the includes in a single place to maintain a clear overview of what is being included in which order.

For a baseline/starter framework like this, it makes more sense to be more explicit, so as to give a better understanding to new users/developers.

Implemented C-style comment blocks and hierarchy. Improved documentation of blocks at the same time.

Fixes pr#3.

Implemented Clean CSS to improve compression. Moved supported browsers to package.json as recommended by browserslist library which autoprefixer uses. Implemented gulp-replace to add copyright notice generated by package.json file. Extends pr#6.

Previously all in one file, now in separate files per subject. Includes moving box-sizing from _document into own file under /elements.

Added missing comma.

59f911b

LucaPipolo requested a review from colourgarden August 25, 2017 09:27

sun reviewed Aug 25, 2017

View reviewed changes

LucaPipolo force-pushed the proposed-changes-luca branch from a9db5cc to 41c043a Compare August 25, 2017 09:59

colourgarden reviewed Aug 31, 2017

View reviewed changes

colourgarden mentioned this pull request Sep 1, 2017

Implement clean-css to minify dist files. #4

Closed

sun reviewed Sep 1, 2017

View reviewed changes

colourgarden reviewed Sep 6, 2017

View reviewed changes

sun approved these changes Sep 6, 2017

View reviewed changes

colourgarden and others added 10 commits September 7, 2017 11:30

Added SassDoc annotations on functions and mixins.

320863b

Added hypenated BEM pattern to Stylelint rules.

dc1b1f7

Removed legacy normalize.css file.

4ec98b2

Fixed comment style and expanded documentation.

1d6cd63

Implemented C-style comment blocks and hierarchy. Improved documentation of blocks at the same time.

Added code block styling to docs.

441f87f

Updated package.json values.

2356cad

Fixes pr#3.

Changed base element styling to be separate files.

d8dd715

Previously all in one file, now in separate files per subject. Includes moving box-sizing from _document into own file under /elements.

Changed import of forms stylesheets.

627a05d

Fixed comments.

7a0219a

colourgarden force-pushed the proposed-changes-luca branch from 30987b5 to 7a0219a Compare September 7, 2017 09:47

colourgarden merged commit 8683987 into master Sep 7, 2017

LucaPipolo deleted the proposed-changes-luca branch November 2, 2018 17:00

Proposed changes luca #2

Proposed changes luca #2

Conversation

LucaPipolo commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucaPipolo Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colourgarden commented Sep 1, 2017

LucaPipolo commented Sep 1, 2017

LucaPipolo commented Sep 1, 2017

sun left a comment

Choose a reason for hiding this comment

sun Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucaPipolo Aug 25, 2017 •

edited

Loading

sun Sep 1, 2017 •

edited

Loading