Rework Sass-Styleguide #2

rastersysteme · 2017-06-21T15:14:18Z

No description provided.

rastersysteme · 2017-06-21T15:15:54Z

@ChristianPeters Ich habe das ganze ein wenig angepasst und erweitert. Magst du schon mal drüber schauen und mir Feedback geben bzw. sagen, was dir noch fehlt?

ChristianPeters · 2017-06-21T15:27:19Z

sass-style-guide.md

-  3. *Top-to-bottom*: You go on top-to-bottom just how you would read that page
-* Some things are shared, like a variable definition that is referred in multiple parts of your stylesheet. Just put it on top of the first block that uses it. A situation that you should avoid is sharing variables across files. If you cannot avoid that, put it on top of the file.
-* Nestings within the namespace of an element start with pseudo classes, followed by pseudo elements, followed by modifiers, followed by direct children (`>`), followed by children further down the DOM tree (if even necessary). Example:
+Bei der Arbeit an großen, langlebigen Projekten mit mehreren Entwicklern unterschiedlicher Fähigkeiten ist es wichtig, dass wir alle einheitlich arbeiten. Unsere oberste Prämisse sollte sein, dass unsere Stylesheets auf der einen Seite leicht pflegbar und skalierbar sind und auf der anderen Seite unser Code aber transparent und lesbar bleibt. Um dies zu erfüllen, helfen folgende Empfehlungen.


Bei "unterschiedlicher Fähigkeiten" könnte man sich angegriffen fühlen: "Oh, meint er mich damit, bin ich ihm nicht gut genug? "

warum "aber transparent und lesbar"?

Empfehlungen? Wenn du das so sagst, ist es auch in Ordnung es anders zu machen. "Guidelines"?

@ChristianPeters Ich wollte ein deutsches Wort für Guidelines nehmen und da klang Empfehlungen freundlicher als Handlungsanweisungen. Was hältst du von Richtlinien oder Vorgaben?

Wir nutzen in unserem Sprachgebrauch doch auch das englische Wort "Guidelines". Richtlinien wäre die beste Übersetzung, aber das ist weniger positiv konnotiert.

ChristianPeters · 2017-06-21T15:29:29Z

sass-style-guide.md

+
+Wann wird wo was definiert? Wir gehen inhaltsgetrieben vor, d.h. wir schreiben unseren Sass-Code in der Reihenfolge, wie die Elemente auf der gerenderten Seite erscheinen werden:
+
+1. **von außen nach innen**: ZASAF folgend beginnt man in der Regel mit dem Page-Wrapper-Element.


Was für ein Page-Wrapper-Element? Das kommt doch noch aus meinem alten System, darüber hat in diesen Guidelines ja noch niemand etwas gesagt.

@ChristianPeters Es gibt in der Regel immer ein Page-Wrapper-Element, in das die Komponenten gepackt werden. Bei mir ist das .page.

Aber das steht ja nur in einer gesonderten Page-Komponente und es trifft nicht mehr auf spezifische Komponenten zu. Aber okay, man versteht das Prinzip.

ChristianPeters · 2017-06-21T15:32:39Z

sass-style-guide.md

+  &.highlight
+    bacground-color: $color-background-loud
+
+.main-navigation--item


Wie jetzt, ein zweiter Block zu .main-navigation--item?

Copy/Paste-Fehler :/

ChristianPeters · 2017-06-21T15:33:30Z

sass-style-guide.md

+    border-color: $color-border-loud
+  &::before
+    content: '*'
+  &.highlight


Das ist doch ein Modifier, oder? Dann sollte er .highlighted heißen.

Good catch!

ChristianPeters · 2017-06-21T15:34:07Z

sass-style-guide.md

+.main-navigation--item
+  display: block
+  padding: 5px 0
+  .highlight > &


Was ist .highlight? Es klingt inhaltlich wie ein Modifier und sollte dann nicht allene stehen.

@ChristianPeters Ja, .highlighted ist ein Modifier. Aber warum darf er nicht alleine stehen?

Variante 1:

.main-navigation--item &.highlighted bacground-color: $color-background-loud .main-navigation--link .highlighted > & color: $color-text-inverted

vs.

Variante 2:

.main-navigation--item &.highlighted bacground-color: $color-background-loud .main-navigation--link .main-navigation--item.highlighted > & color: $color-text-inverted

vs.

Variante 3:

.main-navigation--item &.highlighted bacground-color: $color-background-loud .main-navigation--link color: $color-text-inverted .main-navigation--link …

Bei Variante 3 steht die Eigenschaft nicht beim Link, wo sie meines Erachtens hingehört. Daher ist das die schlechteste Vorgehensweise. Bei Variante 2 überqualifiziere ich den Selektor, den ich später im CSS nicht brauche. Daher bin ich für Variante 1.

Das ist eine Grundregel von Modifiern, s. https://github.com/zweitag/html-css-guidelines/blob/master/README.md#modifier:

steht nie alleine, sondern ist nur in Verbindung mit der Komponente eindeutig

Ein Modifier an sich ist eben nicht eindeutig. Es können viele Dinge .highlighted sein. Nun ist die .main-navigation immer sehr hoch in der Seitenhierarchie angesiedelt. Aber nimm eine beliebige Liste, die gehighlightete Einträge enthalten kann, die wiederum Inhalt enthalten kann, der gehighlighted sein kann:

%ul.articles-list %li.articles-list--item.highlighted %h3 Hitlers geheimes Silberbesteck gefunden %ul.tag-cloud %li.tag-cloud--item.highlighted %a.tag-cloud--link Hitler %li.tag-cloud--item %a.tag-cloud--link Sommerflautenunfug %li.articles-list--item.highlighted %h3 Große Dürre: Werden wir alle sterben? %ul.tag-cloud %li.tag-cloud--item %a.tag-cloud--link Wetter %li.tag-cloud--item %a.tag-cloud--link Sommerflautenunfug

Wenn du jetzt schreibst…:

.tag-cloud--link &.highlighted background-color: yellow

… dann sind in obigem Beispiel alle Tag-Cloud-Links gelb hinterlegt, obwohl es nur der mit "Hitler" sein sollte.

Daher die Regel: Modifier dürfen nie alleine stehen. Alternativ müsste man es m.M.n. so machen wie BEM und %a.tag-cloud--link.tag-cloud--link__highlighted schreiben oder sowas.

Variante 2 ist für mich die richtige, da der Selektor demnach nicht überqualifiziert ist. Zu Variante 3 gebe ich dir Recht, finde ich auch ungünstiger.

Die Grundregel

steht nie alleine, sondern ist nur in Verbindung mit der Komponente eindeutig

habe ich eher so verstanden, dass ich folgendes nicht machen darf:

.highlighted background-color: yellow

In meinem Beispiel steht .highlighted demnach auch nicht alleine, sondern bezieht das Nachfahrenelement mit ein. Apropos, Ich nehme an, in deinem obigen Sass-Beispiel meinst du eigentlich:

.tag-cloud--link .highlighted & background-color: yellow

In dem Fall versteh ich deinen Einwand, das wäre Mist. Wenn du dir mein Beispiel oben aber nochmal näher anschaust, dann habe ich ein > benutzt, was das Problem löst.

Ich finde übrigens auch, dass

.tag-cloud--link .tag-cloud--item.highlighted & background-color: yellow

schwerer zu lesen ist. Der Modifier, auf den es mir letztendlich ankommt, geht unter.

Und ja, an dieser Stelle wünscht man sich BEM. Hier würde der hervorgehobene Link eindeutig mittels tag-cloud__link--highlighted selektiert werden können. Ach, wie war das nochmal mit CSS Modules? ;)

Okay, > löst genau dieses Problem hier technisch. Aber wie einfach ist das mal zu übersehen oder zu vergessen? Auch wenn es dir nicht passieren würde, es ist schon subtil und wenn man sich der Tragweite nicht bewusst es, kommt der Autor nicht drauf es zu schreiben und ein Reviewer übersieht es vielleicht beim Durchgucken.

Und was machst du, wenn da mehrere Elemente noch dazwischenstehen?

Hier bin ich der Meinung, dass du hier zu sehr für diesen Fall optimierst zu einem hohen Preis. Einfache Regeln sind sehr wertvoll. "darf nie" ist sehr viel einfacher als "soll nicht, es sei denn …".
Wenn du schon zustimmst, dass man sich an dieser Stelle BEM wünscht, dann spricht das doch auch für die explizitere Variante 2?

Auch wenn ich aufgrund von Lesbarkeit und weniger CSS-Code immer noch meine Variante bevorzuge, so ist das Argument mit den Elementen dazwischen kein so schlechtes wenn ich ehrlich bin. Ich könnte jetzt zwar ausholen und sagen, dann muss man eben die Architektur umstricken, aber das führt zu weit. Ich will endlich fertig werden hier. Daher können wir es gern zu handhaben. Ich würde den PR #3 gleich mergen.

Es geht mir vor allem darum, das System nicht zu kompliziert zu machen. Je ausgefeilter das Regelwerk desto unwahrscheinlicher ist eine 100%-Einhaltung durch alle Anwender.

Wenn in einer Diskussion eine Tradeoff-Entscheidung zwischen mehreren Varianten getroffen werden muss (Explizierung dessen was gemeint ist vs. dadurch entstehendes visuelles Rauschen), und die unterschiedlichen Positionen aus einer anderen Gewichtung der Präferenzen entsteht, find ich es gut, wenn im Zweifel die einfachere Lösung gewinnt.

Und hier würde ich sagen: "Bei Modifier immer Selektor von der modifizierten Komponente davor" ist einfacher als wenn man darauf achten muss, Kindselektoren einsetzbar zu machen.

ChristianPeters · 2017-06-21T16:34:16Z

sass-style-guide.md


-It’s the typographic standard to use number keywords. Also it’s shorter and saves some bits.
+Die Zahlen 100, 200 … 900 sind typografischer Standard. Außerdem sind sie kürzer und sparen ein paar Bits ein. ;)


Das 1 Zeichen ist lächerlich. An anderer Stelle erlauben wir uns viel inperformantere Praktiken, die auch nicht Erwähnung finden. Triftiger ist doch das Argument, dass man mit der Zahlen-Darstellung besser differenzieren kann und auch nicht mal "bold" und mal "600" schreiben möchte.

ChristianPeters · 2017-06-21T16:35:52Z

sass-style-guide.md


-We want to make it easy for others to find their way through stylesheets. Therefore, we group rules and give them a title by writing a comment. We distinguish 3 types of comments:
+Zum Testen unseres Sass-Codes benutzen wir [Sass Lint](https://github.com/sasstools/sass-lint). Der Linter sollte am Ende keine Fehler oder Warnungen ausspucken.


Ende = Projektende = Projekt wird übergeben oder eingestampft und dann ist es uns eh egal? :) Hier sind genauere Richtlinien von Nöten, wann der Linter ohne Fehler und wann ohne Warnungen durchzulaufen hat plus Best Practices

@ChristianPeters Hierüber hatten wir ja schon gesprochen, Stichwort Commit-Hook/CI-Test. Ich kann den letzten Satz auch erstmal streichen, wenn du dich so sehr daran störst. Bez. der Richtlinie könnte man natürlich einfach sagen, dass es zu keinem Zeitpunkt Fehler oder Warnungen geben darf (wobei mich mich gerade frage, wozu dann überhaupt Warnungen!?).

ChristianPeters · 2017-06-21T16:36:39Z

sass-style-guide.md

-//=============
+Möchte man eine einzelne Zeile in einem Block kommentieren, so setzt man den Kommentar ans Ende der Deklaration:
+
+```


Syntax-Highlighting fehlt

ChristianPeters · 2017-06-21T16:36:44Z

sass-style-guide.md


-.photo-gallery
-  …
+```


Syntax-Highlighting fehlt

ChristianPeters · 2017-06-21T16:37:58Z

sass-style-guide.md


-// Video Styles
-//=============
+## Editorconfig


Typo: EditorConfig

…pic 'selector rules'

rastersysteme · 2017-06-22T07:54:25Z

@ChristianPeters Ich bin mit dem ersten Einarbeiten deines Feedbacks durch, habe hier und da aber noch Rückfragen bzw. Disksussionsbedarf. ;)

ChristianPeters · 2017-09-06T12:03:57Z

sass-style-guide.md

+  &::before
+    content: '*'
+  &.highlighted
+    bacground-color: $color-background-loud


ChristianPeters · 2017-09-06T12:25:51Z

sass-style-guide.md

+.main-navigation--item
+  display: block
+  padding: 5px 0
+  .highlight > &


Okay, > löst genau dieses Problem hier technisch. Aber wie einfach ist das mal zu übersehen oder zu vergessen? Auch wenn es dir nicht passieren würde, es ist schon subtil und wenn man sich der Tragweite nicht bewusst es, kommt der Autor nicht drauf es zu schreiben und ein Reviewer übersieht es vielleicht beim Durchgucken.

Und was machst du, wenn da mehrere Elemente noch dazwischenstehen?

Hier bin ich der Meinung, dass du hier zu sehr für diesen Fall optimierst zu einem hohen Preis. Einfache Regeln sind sehr wertvoll. "darf nie" ist sehr viel einfacher als "soll nicht, es sei denn …".
Wenn du schon zustimmst, dass man sich an dieser Stelle BEM wünscht, dann spricht das doch auch für die explizitere Variante 2?

ChristianPeters

Es sind noch 2 Themen und 2 kleine To-Dos offen:

Themen:

Modifier dürfen nie alleine stehen – oder manchmal doch?
Linter: Verwendungsregel und -hilfe (Config)

Offene To-Dos:

"Guidelines" statt "Empfehlungen"
Typo (s.o.)

rastersysteme · 2017-09-06T13:32:32Z

@ChristianPeters

Linter: Verwendungsregel und -hilfe (Config)

Das würde ich im Sass Linting PR im anderen Repo gern abhandeln. Dort ist ja auch noch ein Kommentar von dir offen, den ich angehen muss. Wenn das für dich OK ist, kannst du den hier gern mergen.

ChristianPeters · 2017-09-06T16:55:34Z

sass-style-guide.md

+.main-navigation--link
+  display: block
+  padding: 5px 0
+  .highlighted > &


Änderst du das dann bitte noch?

ChristianPeters · 2017-09-06T17:00:48Z

sass-style-guide.md

-//--------------------
-.photo-contest-panel
-  …
+Zum Testen unseres Sass-Codes benutzen wir [Sass Lint](https://github.com/sasstools/sass-lint). Der Linter sollte am Ende keine Fehler oder Warnungen ausspucken.


Nach wie vor ist noch undefiniert, wann gelintet werden soll. Das steht auch im Rails Template PR nicht. Wann machst du das denn? Hast du einen Pre-Commit-Hook? Gibt's nen Tool, um es als PR-Check hinzuzufügen?

Hier fehlt dann auch noch der Link zur Config

rastersysteme · 2017-09-07T07:31:34Z

Derzeit linte ich meine Sass-Dateien automatisch in Sublime Text, damit sie noch vor dem Comitten korrekt ins Repo wandern. Man könnte auch überlegen, ob man Travis das testen lässt. Was ist da deine Präferenz?

ChristianPeters · 2017-09-07T08:29:01Z

Derzeit linte ich meine Sass-Dateien automatisch in Sublime Text, damit sie noch vor dem Comitten korrekt ins Repo wandern. Man könnte auch überlegen, ob man Travis das testen lässt. Was ist da deine Präferenz?

Ich glaube, man braucht beides: sowohl eine lokale Lösung, zumindest für Frontendler, damit die Fehler frühestmöglich erkannt werden als auch einen PR-Check, um sicherzugehen, dass keine Violations in Master kommen.

PR-Check könnte Travis CI sein oder ein extra Dienst, der die Violations z.B. als Kommentare an entsprechende Code-Zeilen schreibt. Ich glaube, das ist im Endeffekt praktischer als ein failender Travis-Build. Kostet mit ca. 10$/Repo aber auch ordentlich. Primärer Anbieter wäre https://houndci.com/, nicht wirklich günstigere Alternative https://sideci.com/ und dann noch das "Ungetüm" CodeClimate.

@hzeus @stsc3000 Wie weit seid ihr damit bei Ruby?

@rastersysteme Willst du vielleicht mal bei DZF die TravisCI-Konfig ausprobieren? Vielleicht kommen wir ja auch damit aus.

hzeus · 2017-09-07T08:41:33Z

Wir haben bei Ruby noch keine eine automatisierte PR-Lösung geplant, das läuft im Moment noch auf freiwilliger Basis auf den Entwickler-Rechnern.

Wenn ihr für sass eine Lösung sucht, wäre es natürlich sinnvoll, wenn die direkt beides kann.

rastersysteme · 2017-09-07T08:43:49Z

Mein Vorschlag wäre auch Hound CI. So hätte man auch eine Lösung für Rubocop.
Ich kann das aber gern einmal bei DZF mit Travis ausprobieren.

@rastersysteme

In Absprache mit @rastersysteme

ChristianPeters · 2017-09-07T13:51:01Z

Okay, kann jetzt gemergt werden.

Die Struktur des Repos ist aber dadurch noch nicht ganz im Reinen. Wir haben auf der Startseite auch noch Selector Best Practices. Ich denke, es muss nicht alles hier in der einen Datei sein.
Bei den Selector Best Practices verweisen wir noch auf externe Links. Darüber hatten wir ja mal gesprochen, oder? Während ich es gut finde, Räder nicht neu zu erfinden, sollte man doch auch Content lieber im Sinne eines Zitats übernehmen, wo dann aber auch klar ist, was davon gutgeheißen wird und was nicht. Die Quelle sollte dann freilich verlinkt werden.

Ich lege dafür mal Issues an.

Rework Sass-Styleguide

6f389f3

rastersysteme self-assigned this Jun 21, 2017

ChristianPeters requested changes Jun 21, 2017

View reviewed changes

rastersysteme added 11 commits June 22, 2017 07:30

Add linked items to the table of contents

7811081

Delete unnecessary words

a1205b4

Fix modifier naming

f60898c

Rewrite syntax and formatting intro sentence

d9eb2fb

Add nesting depth explanation

8f8eaa0

Move parts from 'syntax and formatting' into their own topics, add to…

fb07bf9

…pic 'selector rules'

Rename 'Elementselektor' to 'Basiselementselektor'

2b78a30

Adjust shorthand properties description

75ea027

Adjust font-weight description

8976470

Add syntax highlighting for code blocks

5b0eb94

Fix typo

6015fae

rastersysteme added 2 commits June 22, 2017 15:42

Adjust wording

4fee27e

Remove ID option for adding JS behaviour to en element

3e6458f

ChristianPeters reviewed Sep 6, 2017

View reviewed changes

ChristianPeters requested changes Sep 6, 2017

View reviewed changes

rastersysteme added 3 commits September 6, 2017 15:02

Fix typo

d7e5ff3

Change wording

d386d3f

Add link to Sass Linting PR

18ac409

ChristianPeters requested changes Sep 6, 2017

View reviewed changes

rastersysteme added 3 commits September 7, 2017 09:07

Define modifier only in conjunction with its element

56e2630

Add link to sass linter config file

f408a75

Add link to linter rules documentation

08de0b2

Add linter installation and usage guide

9052af1

Stelle Linter-Verwendungsrichtlinien klar

25f91e5

In Absprache mit @rastersysteme

ChristianPeters approved these changes Sep 7, 2017

View reviewed changes

rastersysteme merged commit 28e3fd5 into master Sep 7, 2017

rastersysteme deleted the rework-sass-style-guide branch September 7, 2017 13:50


		Wann wird wo was definiert? Wir gehen inhaltsgetrieben vor, d.h. wir schreiben unseren Sass-Code in der Reihenfolge, wie die Elemente auf der gerenderten Seite erscheinen werden:

		1. von außen nach innen: ZASAF folgend beginnt man in der Regel mit dem Page-Wrapper-Element.


		It’s the typographic standard to use number keywords. Also it’s shorter and saves some bits.
		Die Zahlen 100, 200 … 900 sind typografischer Standard. Außerdem sind sie kürzer und sparen ein paar Bits ein. ;)


		We want to make it easy for others to find their way through stylesheets. Therefore, we group rules and give them a title by writing a comment. We distinguish 3 types of comments:
		Zum Testen unseres Sass-Codes benutzen wir [Sass Lint](https://github.com/sasstools/sass-lint). Der Linter sollte am Ende keine Fehler oder Warnungen ausspucken.

Rework Sass-Styleguide #2

Rework Sass-Styleguide #2

Conversation

rastersysteme commented Jun 21, 2017

rastersysteme commented Jun 21, 2017

ChristianPeters Jun 21, 2017 • edited by rastersysteme Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianPeters Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianPeters Jun 21, 2017 • edited by rastersysteme Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianPeters Jun 21, 2017 • edited by rastersysteme Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianPeters Jun 21, 2017 • edited by rastersysteme 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

ChristianPeters Jun 21, 2017 • edited by rastersysteme Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianPeters Jun 21, 2017 • edited by rastersysteme Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianPeters Jun 21, 2017 • edited by rastersysteme Loading

Choose a reason for hiding this comment

ChristianPeters Jun 21, 2017 • edited by rastersysteme Loading

Choose a reason for hiding this comment

ChristianPeters Jun 21, 2017 • edited by rastersysteme Loading

Choose a reason for hiding this comment

rastersysteme commented Jun 22, 2017

ChristianPeters Sep 6, 2017 • edited by rastersysteme Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianPeters left a comment

Choose a reason for hiding this comment

rastersysteme commented Sep 6, 2017

ChristianPeters Sep 6, 2017 • edited by rastersysteme Loading

Choose a reason for hiding this comment

ChristianPeters Sep 6, 2017 • edited by rastersysteme Loading

Choose a reason for hiding this comment

rastersysteme commented Sep 7, 2017

ChristianPeters commented Sep 7, 2017

hzeus commented Sep 7, 2017

rastersysteme commented Sep 7, 2017 • edited Loading

ChristianPeters commented Sep 7, 2017

ChristianPeters Jun 21, 2017 •

edited by rastersysteme

Loading

ChristianPeters Jun 21, 2017 •

edited

Loading

ChristianPeters Jun 21, 2017 •

edited by rastersysteme

Loading

ChristianPeters Jun 21, 2017 •

edited by rastersysteme

Loading

ChristianPeters Jun 21, 2017 •

edited by rastersysteme

Loading

ChristianPeters Jun 21, 2017 •

edited by rastersysteme

Loading

ChristianPeters Jun 21, 2017 •

edited by rastersysteme

Loading

ChristianPeters Jun 21, 2017 •

edited by rastersysteme

Loading

ChristianPeters Jun 21, 2017 •

edited by rastersysteme

Loading

ChristianPeters Jun 21, 2017 •

edited by rastersysteme

Loading

ChristianPeters Sep 6, 2017 •

edited by rastersysteme

Loading

ChristianPeters Sep 6, 2017 •

edited by rastersysteme

Loading

ChristianPeters Sep 6, 2017 •

edited by rastersysteme

Loading

rastersysteme commented Sep 7, 2017 •

edited

Loading