🎉 Switch to SASS #18

enzy · 2017-03-07T12:36:52Z

whitespace syntax
using normalize-scss
using Bourbon mixins
$manifest variable is gone
mango-cli v0.30+

Closes #17

- $manifest is missing as json read is not supported yet

JirkaVebr

Pěkně! 🙂

Jinak nepřidáme ještě ty position mixiny? Ten size už dodává Bourbon a same byl spíš takovej experiment, ale ty position mixiny jsou podle mě lepší, než Bourboní position.

- with usage examples

FilipChalupa · 2017-03-07T14:07:41Z

src/styles/global.sass

+	clip: rect(1px, 1px, 1px, 1px)
+	overflow: hidden
+	size: 1px 1px
+.fleft


K čemu tu vlastně stále máme .fleft, .fright a .clear? Nepřišlo mi to užitečné dříve a nepřijde mi to ani teď. Akorát si tím rozbíjíme koncept modulárního css.

a .accessible ti nevadí? Je to stejný princip utilitek, které se nevyplatí psát přímo do komponenty

Já souhlasím s tím, že fleft, fright a clear nejsou úplně sémantické. Zrušil bych je a z .accessible udělal extend-only selector (nebo klidně mixin) a pak ho explicitně volal z komponent.

Kdyby .accessible bylo v parts nebo v abstract pro jednoduchý @extend, tak jsem s ním úplně spokojený. Je to k něčemu dobré a skoro to respektuje modulární css (vadí mi jen umístění do global).

Vzhledem k tomu, že v našich HTML má téměř každý element třídu, .fleft, .fright a .clear neusnadňuje nic, protože místo toho stačí napsat jen float left, float right a clear both, což je sice o pár znaků delší, ale je to známým jazykem, na očekávaném místě a podporuje to modifikace třeba v breakpointech. .fleft mi přijde podobně hloupé jako třeba dflex pro display flex, který navíc používáme mnohem častěji.

Proč .fleft a ostatní chceme? Ještě jsem na jejich použití v žádném codereview nenarazil.

FilipChalupa · 2017-03-07T14:10:41Z

src/styles/parts/appName.sass

@@ -0,0 +1,3 @@
+.appName
+	padding-left: 0.3em
+	border-left: 0.3em solid $c-main


To, že jsi zahodil manifest, mě dost mrzí. :( Co já vím, tak přítomnost manifestu například zvyšuje hodnocení v Googlu. Čím více ten manifest integrujeme do zbytku examplu, tím je jistější, že bude v každém projektu z něj vycházejícím dostatečně udržovaný.

FilipChalupa · 2017-03-07T14:16:53Z

src/styles/parts/shape.sass

+
+	// Shapes default dimensions
+	&-plus
+		+size(20px)


Proč řádek 84cd298#diff-9ca1fe03ff873ff03f519cea30829fb7R75 není zapsaný stejně jako tady?

není, mohl by

FilipChalupa · 2017-03-07T14:19:50Z

src/styles/variables.sass

+$portrait: '(orientation: portrait)'
+
+// Animations
+@keyframes spin


Nechceme pro animations vlastní složku na na úrovni comps a parts? Řekl bych, že na Darujme se nám to osvědčilo.
Cc. @JirkaVebr

FilipChalupa · 2017-03-07T14:21:31Z

src/styles/variables.styl

@@ -1,58 +0,0 @@
-// App manifest
-$manifest = json('../data/manifest.json', { hash: true })


Co se ti na tomhle nelíbí?

SASS to neumí. Plugin sice existuje, ale použití proměnných je pak úplně jiné.

Tušil jsem, že jde jen o zjednodušení. Návrat $manifest si tedy beru jako issue na sebe.

FilipChalupa · 2017-03-07T14:23:44Z

src/styles/variables.sass

@@ -47,6 +47,11 @@ $break1200M: '(max-width: 1199px)'
 $landscape: '(orientation: landscape)'
 $portrait: '(orientation: portrait)'

+// Mixins
+=break($min)


FilipChalupa · 2017-03-07T14:25:33Z

src/styles/variables.sass

+	+position(relative, $properties)
+
+// +sticky
+=sticky($properties: null null null null)


❤️
Sticky nib/stylus neumí.

FilipChalupa · 2017-03-07T14:30:18Z

src/styles/variables.sass

@@ -48,10 +48,16 @@ $landscape: '(orientation: landscape)'
 $portrait: '(orientation: portrait)'

 // Mixins
+
+// +break(768) => @media (min-width: 768px)
 =break($min)


Nechceme zahodit ty konstantní deklarace? $break320: '(min-width: 320px)', ...

JirkaVebr · 2017-03-07T14:20:26Z

src/styles/variables.sass

+// +media($break320M) => @media (max-width: 319px)
+=media($var)
+	@media #{$var}
+		@content


Úplně se mi nelíbí, že jsou takhle dva separátní mixiny, a že navíc chybí něco jako breakM, tak jsem napsal univerzální řešení. Je to pohodlnější, flexibilnější a líp se s tím pracuje za použití proměnných. Klidně o tom diskutujme a společně to vylepšeme (zejména s tou magií s M mnozí klidně můžou mít problém), ale podle mě není důvod mít takhle dva separátní spíše kostrbaté mixiny.

Wow. :)

Hele, proč je tam $literal * 0? Co to dělá?

Je to takovej trik na zbavení se jednotky, pokud tam nějaká je. $literal * 0 udělá z 123px 0px. Pak to + 1 udělá z 0px 1px a $literal / 1px, tedy 123px / 1px, je 123.

Dík. :)

Co to 123px / 1px? Zbyde jen 123, protože jednotky se vykrátí? :D

Ano, přesně tak ‒ funguje to úplně stejně jako ve fyzice. Podobně jako 2px * 3px je v sassu 6px*px. 😃

Oukej. :D :D

Tohle byla co nejjednodušší rychlovka... Codepen vypadá fajn, co k tomu přidat nějakou dokumentaci, ať to umí pak každej používat? :)

JirkaVebr · 2017-03-07T14:27:40Z

src/styles/variables.sass

+
+// +absolute(0 10px 0 10px), order by specs: top right bottom left
+=absolute($properties: null null null null)
+	+position(absolute, $properties)


Proč Ti prosím přijde tohle lepší než to mnou navržené řešení? Osobně s tím moc nesouhlasím ‒ nemyslím si, že je +absolute(null 10px null 0) jakkoliv čitelnější či pohodlnější než +absolute(right 10px, left). Navíc jsme na to byli z nibu zvyklí.

více psaní, vyhledatelnost chování v dokumentaci, shorthand zápis dle zvyků v CSS

Hmm, fair enough. 🙂 Zmátl mě ten příklad +absolute(0 10px 0 10px) ‒ myslel jsem, že to znamená, že člověk vždycky musí takhle psát všechny čtyři hodnoty. Ono ale funguje i +absolute(0 10px), a to pak dává smysl. Mea culpa.

JirkaVebr · 2017-03-07T14:33:36Z

src/styles/variables.sass

+
+// +sticky
+=sticky($properties: null null null null)
+	+position(sticky, $properties)


Jinak podle mě by bylo lepší mixiny dát do separé souboru.

Už mi také přišlo, ale ještě jsem neřešil.

JirkaVebr · 2017-03-07T17:54:56Z

@enzy Já se domnívám, že ne. Mít to ve variables je bordel a dávat je do parts je jen spoléhání se na to, že jméno animace je abecedně dřív než jméno komponent, které by tu animaci rády využily. Těch animací bude sice pár, ale i tak je tohle podle mě nejlepší řešení.
Edit: reaguju na ty animace, of course… 🙂

FilipChalupa · 2017-03-07T17:57:51Z

@JirkaVebr Animace se do parts nehodí, ale jinak jim nevadí, jestli keyframes mají zadefinované před nebo až po vlastním použití, takže na abecedě nezáleží.

JirkaVebr · 2017-03-07T17:59:16Z

@onset Máš pravdu, to jsem si spletl s abstract. I tak bych je ale do variables.sass ani přímo do styles/ nedával.

enzy

Reasoning za vlastním souborem: Přehled všech možností ve file tree, předcházení konfliktů, přehlednost v souboru.
Animace jsou ale max. na pár řádků a nepoužívá se jich na projektech tolik, aby v nich začal být nepořádek a potřebovali jsme toto řešení.

Mixiny si naopak zaslouží vlastní dokumentaci i příklady, jak to Jirka pěkně navrhl, takže tam je to validní.

FilipChalupa · 2017-03-07T18:09:35Z

@enzy Zmínil jsi jen samé výhody, proč mít něco ve vlastním souboru. Má to nějaké nevýhody?

enzy · 2017-03-07T18:12:32Z

Nechceme přejít na reviewable? Tohle začíná být docela dost chaos. A to se nepřidal Vilík a David.

Držme citace při odpovědích a bude to fajn. Rewiable tomuto moc nepomůže.

Zmínil jsi jen samé výhody, proč mít něco ve vlastním souboru. Má to nějaké nevýhody?

@onset Založení souboru a vůbec manipulace se soubory (přejmenování) je výrazně pomalejší, nežli s textem v jednom souboru. Dále narůstá velikosti stromu souborů, která při určitě hranici začíná být kontraproduktivní.

FilipChalupa · 2017-03-07T18:17:55Z

@enzy IMHO, když máš těch souborů pár, tak tě problém s manipulací neobtěžuje, protože prakticky vůbec nemanipuluješ. Jak je těch souborů víc, tak je lepší pro ně, pro animace, mít jednotnou přehlednou strukturu, takže se prostě vyplatí udržovat strukturu i v případě, že animací je málo, protože je to příprava na stav, kdy se jejich množství zvedne.

JirkaVebr · 2017-03-07T18:20:13Z

@enzy Já uvažoval následovně:

Necháme-li animace ve variables, bude v tom bordel, protože tam prostě nepatří, neboť to jednoduše žádné proměnné nejsou.
Dáme-li každou animaci do parts/, bude v tom bordel, protože člověk na první pohled nepozná, co je komponenta, a co je animace. To je zkrátka nepřehledné a to, že těch animací není tolik, na tom imho nic nemění.
Bude-li si každá komponenta definovat vlastní keyframes, je to zbytečně ne-DRY.

Jediná dvě rozumná řešení, která mě napadají, je mít ve styles/ soubor animations.sass nebo to dát do té samostatné složky, jak jsem to teď commitnul. Klidně ty animations.sass vytvořím, to je asi jedno. Ostatně i na Darujme, projektu jako kráva, těch animací máme jen šest. Nevýhodou je, že to bude trochu haprovat, pokud těch animací jednou skutečně bude hodně, nebo pokud se třeba nějaká hodně rozroste. Pak ale asi není problém to operativně přepsat na ten způsob se složkou, ikdyž souhlasím s Fílou, že je potom lepší to tak mít rovnou ‒ ten způsob se složkou je zkrátka univerzálnější. Jen bych prosím rozhodně nešel ani jednou z cest popsaných v těch bodech výše.

enzy · 2017-03-07T18:38:50Z

Ještě prohloubení myšlenky... Až začneme používat CSS variables, kam je budeme umisťovat? Můžeme také namítat, že to nejsou vlastně SASS variables, takže by měly patřit jinam, ať nemícháme jablka s hruškami...
A file tree nám bude bobtnat a bobtnat. Takže je to vždy balanc mezi striktním svazujícím oddělováním a uvolňující benevolencí.

Pokud to opravdu chceme oddělit, prosím do styles/_animations.sass

VojtaStanek · 2017-03-07T22:50:24Z

src/styles/variables.sass

+$em-base: $base-font-size
+
+// Colors
+$c-main: green


Když už se tu dějí věci, tak mi trochu tady některé proměnné překáží. Třeba tyhle barvy. Snad nikdy jsem je nezměnil, protože jsem něvěděl na co a potom to místy bylo na nic.

Umm, tomuhle vůbec nerozumím. Úplně každý web má přece nějakou barevnou paletu, přičemž ty nejdůležitější barvy se objevují hodně často v mnoha komponentách. Je dobré pro ně mít sémantické názvy, ne? Třeba u Rekol by $c-main byla ta růžová, protože tu barvu člověkv kódu potřebuje hodně. Souhlasím, že je mnohdy nesnadné ty barvy sémanticky pojmenovat, ale je to rozhodně lepší než hlavní odstíny hardcodovat (asi u půlky projektů, na kterých jsem kdy dělal, se v průběhu vývoje změnil design…) nebo je pojmenovávat stylem $green.

Za mě $c-main dobré, ale jak správně použít ty další vůbec netuším ($c-comp, $c-alt, $text-color-light - které navíc z nějakého důvodu nezačíná na $c-).

Ten prefix je fail, s tím souhlasim, to už jsem taky chtěl změnit ‒ udělám to za chvilku.
Jinak alt a comp může být legrace, ale jaká je lepší alternativa? $c-gray?

Já nevím, co je alt a comp, ale $c-gray rozhodně není dobré pojmenování nezávisle na tom, co to má být.

Mě proměnné na barvy taky nevadí, než začnu reálně kodovat tak si přesně do variables.styl definuju fonty, barvy, radius atd...různý barvy řeším jako c-primary nebo c-secondary a jejich varianty prefixem -light nebo -dark a na to zpravidla pouzivam mixin např. lighten($c-primary, 10%). Víc základních barev než 2 se většinou nepoužívají, a pokud pro nějaký komponenty jo tak pro dalsi barvy pouzivam c-red apod. Prijde mi to v pohode, protoze ty barvy se na webu opakujou a nikdy jsem se nesetkal se situci ze bych musel z c-red udelat treba modrou. Kdyz uz se neco meni tak je to c-primary resp. c-main. Jinak prefix -alt taky moc nepouzivam, pokud jo tak je to na kontrastni barvu.

comp - complementary color (k main barvě)

alt - alternative color (ala secondary)

text-color- jsou bootstrap proměnné

Trochu mi i comp zní jako secondary. Jaké je prosím pro comp nějaké konkrétní použití?

Aha, už chápu. Complement jako doplněk, jako opak.

VojtaStanek · 2017-03-07T22:51:35Z

src/styles/variables.sass

+$fw-normal: 400
+$fw-medium: 500
+$fw-semibold: 600
+$fw-bold: 700


A k čemu je vlastně tohle? Použil to někdy někdo? Mě přijde i čitelnější když je v kódu font-weight: 700, než s nějakou variable...

Je to v zásadě totéž, jako jsem vysvětloval u media queries ‒ takhle Ti to zařve při překlepu, líp Ti našeptává IDE, nemusíš si přesně pamatovat čísla a změny jsou snáz proveditelné. Ani jednu z těchto výhod psaní konstant nemá.

Čísla si nemusíš pamatovat ani když použiješ bold, normal, ... a IDE ti taky našeptá.

@JirkaVebr Mimochodem na Darujme v intervalu jednoho měsíce používáš $fw-bold i bold.

A co až budeš chtít použít light, semi-bold nebo extra-bold? To budeme mixovat bold a 300? Pak si ta čísla člověk stejně nakonec pamatovat musí, nemá-li proměnné.

na Darujme v intervalu jednoho měsíce používáš

@onset Tak jsi mi to měl omlátit o hlavu při review. To, že nejsem neomylný, tady přece vůbec relevantní není…

Máš point, ale je fakt, že ty proměnné snad vůbec nikdo nepoužívá dobře. Zrovna na Darujme v tom děláš bordel ty, Honza, ale i já, takže ti to těžko můžu omlátit o hlavu, když to sám neovládám. Že v tom děláš chyby byl teď nejdříve jen můj odhad, protože mi to bylo úplně jasné. :D

VojtaStanek · 2017-03-07T22:52:27Z

src/styles/variables.sass

+// Font properties
+$font-family-sans-serif: 'Helvetica Neue', Helvetica, Arial, sans-serif
+$font-family-serif: Georgia, Cambria, 'Times New Roman', Times, serif
+$font-family-monospace: Monaco, Menlo, Consolas, 'Courier New', monospace


A k čemu je tohle? To nechápu využití....

Není to hardcoded do deklarací, ale je to vyčleněo sem, díky čemuž člověk může relevantní věci snadno měnit přímo z variables (kam jinam bys jako první šel, kdyby Ti grafik řekl, že se změnilo písmo?). Používá se to (nepřímo) mimo jiné v global.sass.

FilipChalupa · 2017-03-07T23:45:57Z

src/styles/variables.sass

-$text-back: #FFF
+$c-text: $c-comp
+$c-text-light: #676767
+$c-text-back: #FFF


Já bych ideálně z $c-text-back vyhodil to -text. Nedává mi tam příliš smysl nebo používám tuhle proměnnou špatně, ale vnímám ji obecněji než jen jako pozadí pro text.

hexcross · 2017-03-08T13:54:49Z

src/styles/index.sass

+@import 'variables'
+
+// ## Animations
+@import '_animations'


Proc má tenhle jedinej soubor podtržítko?

Nevim, výše to navrhl @enzy a já se nad tim moc nezamyslel. 😃

V SASS se tak označují partials a tady nám to pěkně odděluje hlavní soubory od vedlejších. Ještě bychom mohli přejmenovat další, ať je to patrné.

Nicméně komponenty a další soubory ve složkách už tak dělat není potřeba.

Další jsou které? variables a global?

- better import position

enzy added 3 commits March 7, 2017 13:33

Switched to SASS with Bourbon mixins

84cd298

- $manifest is missing as json read is not supported yet

variables: em-base for Bourbon em/rem functions

17d7d34

mixins: +break(XXY) for easier media queries

f87b660

enzy requested review from ViliamKopecky, JirkaVebr and FilipChalupa March 7, 2017 12:37

JirkaVebr reviewed Mar 7, 2017

View reviewed changes

enzy added 3 commits March 7, 2017 14:18

mixins: absolute, fixed, relative, sticky using Bourbon position mixin

8363802

mixins: added media

eb9fc8b

- with usage examples

💀 🙀

cabdc1d

FilipChalupa requested changes Mar 7, 2017

View reviewed changes

JirkaVebr suggested changes Mar 7, 2017

View reviewed changes

JirkaVebr requested a review from hexcross March 7, 2017 14:59

JirkaVebr added 4 commits March 7, 2017 17:33

Move the position mixins

fb8a822

Add a smarter media mixin

8f5d1a9

Move animations to a separate folder

037d4d7

Move accessible to abstract and remove unsemantic helper classes

061113a

JirkaVebr approved these changes Mar 7, 2017

View reviewed changes

enzy commented Mar 7, 2017

View reviewed changes

Position: add another example

7209cfa

Move animations to _animations.sass

ecbbeea

FilipChalupa approved these changes Mar 7, 2017

View reviewed changes

VojtaStanek reviewed Mar 7, 2017

View reviewed changes

JirkaVebr added 2 commits March 7, 2017 23:33

Standardize color variable name prefix

123042c

Add a var for fw-light

4de2d47

FilipChalupa reviewed Mar 7, 2017

View reviewed changes

FilipChalupa approved these changes Mar 7, 2017

View reviewed changes

hexcross reviewed Mar 8, 2017

View reviewed changes

enzy added 2 commits March 8, 2017 15:12

styles: partials under /styles has underscore prefix

7ea009f

- better import position

variables: c-comp is computed from c-main

faa0876

enzy merged commit faa0876 into master Mar 8, 2017

enzy deleted the sass branch December 14, 2017 10:15

		@@ -1,58 +0,0 @@
		// App manifest
		$manifest = json('../data/manifest.json', { hash: true })

🎉 Switch to SASS #18

🎉 Switch to SASS #18

Conversation

enzy commented Mar 7, 2017

JirkaVebr left a 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

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

JirkaVebr commented Mar 7, 2017 • edited

FilipChalupa commented Mar 7, 2017

JirkaVebr commented Mar 7, 2017

enzy left a comment

Choose a reason for hiding this comment

FilipChalupa commented Mar 7, 2017

enzy commented Mar 7, 2017 • edited

FilipChalupa commented Mar 7, 2017

JirkaVebr commented Mar 7, 2017

enzy commented Mar 7, 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

hexcross Mar 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FilipChalupa Mar 8, 2017 • edited

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

JirkaVebr commented Mar 7, 2017 •

edited

enzy commented Mar 7, 2017 •

edited

hexcross Mar 8, 2017 •

edited

FilipChalupa Mar 8, 2017 •

edited