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

adds icons & compilation script #145

Merged
merged 20 commits into from May 15, 2018
Merged

adds icons & compilation script #145

merged 20 commits into from May 15, 2018

Conversation

janmichek
Copy link

Fixes #144

@janmichek
Copy link
Author

@ujovlado muzes mi pls poradit proc neprochazi test? Na lokalu vse ok, nedokazu potvrdit umisteni spritu na serveru. Snazil jsem se jeste tu kompilaci explicitne uvest do build scriptu, ale zrejme to ten soubor fakt nevidi. f7c2df3

@ujovlado
Copy link
Member

ujovlado commented May 2, 2018

No nevie to najst ten subor s ikonkami:

FAIL  src/indigo/components/AlertBlock.test.js
  ● Test suite failed to run
    Cannot find module '../../../public/icons/symbol/svg/sprite.symbol.svg' from 'Icon.jsx'
      
      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:179:17)
      at Object.<anonymous> (src/indigo/components/Icon.jsx:2:21)

@janmichek
Copy link
Author

CHapu, v tom pripade musi byt nejaka chyba na serverovy casti. Na lokale mi to slo, na soubory na serveru nevidim

@@ -0,0 +1,20 @@
import React from 'react';
import IconSprite from '../../../public/icons/symbol/svg/sprite.symbol.svg';
Copy link
Member

Choose a reason for hiding this comment

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

tato cesta je ale 100% zle

@ujovlado
Copy link
Member

ujovlado commented May 2, 2018

@janmichek neozrejmil by si trosku, co chces dosiahnut? Chces distribuovat len ten sprite alebo aj ostatne svg ikonky? Kazdopadne ta cesta k tomu sprite nemoze byt z public foldra.

@janmichek
Copy link
Author

Snazim se pristoupit k tomu spritu - ty jednotlivy ikonky jsou mi jedno

@ujovlado
Copy link
Member

ujovlado commented May 2, 2018

No a keby sme zaverzovali aj ten sprite? Alebo ako casto sa to bude menit?

@janmichek
Copy link
Author

Klido, muze byt. Nebude se to menit casto, a pokud jo, tak bude duvod to odverzovat

@ujovlado
Copy link
Member

ujovlado commented May 2, 2018

Rozmyslam nad tym takto:

  • v src/icons by boli vsetky ikonky
  • bezal by dev task + watch na tieto ikonky
  • akonahle nieco zmenis tak by sa to pregenerovalo do src/indigo/img/icons-sprite.svg
  • potom by sa to normlane commitlo (tu je ale maly priestor na chybu ak to clovek zabudne)
  • linkoval by si to odtial (src/indigo/img/icons-sprite.svg)

@janmichek
Copy link
Author

Chapu, pokud to neumime udelat ciste na serveru, tak asi nezbyva nez to udelat takhle, jak pises.

@ujovlado
Copy link
Member

ujovlado commented May 2, 2018

Skusil by si to teda upravit? To commitovanie sa mi ale celkom pozdava, lebo je tam lepsia moznost vizulanej kontroly ako na CI (kde je ziadna).

Nie je tam problem "na serveru", len to treba generovat do spravnej cesty.

@janmichek
Copy link
Author

@ujovlado jsem se v tom nejak ztratil, nejde mi updatnout ten test pomoci 'yarn jest -u', jak to fungovalo predtim. Nebo nerozumim tomu co delam blbe. VYpada to ze to svgcko se mu nelibi

@ujovlado ujovlado force-pushed the jan-svg-icons branch 2 times, most recently from 6fbf86a to d57c6fa Compare May 3, 2018 20:55
@ujovlado
Copy link
Member

ujovlado commented May 3, 2018

@janmichek pofixoval som ti to ... este ale prosim pridaj test (+snapshoty) na Icon komponentu a kukni komentare, co som pridal

import IconSprite from './../img/symbol/svg/sprite.symbol.svg';

export default React.createClass({
displayName: 'Icon',
Copy link
Member

Choose a reason for hiding this comment

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

displayName nie je potrebne

iconClass: React.PropTypes.string,
className: React.PropTypes.string,
},
getAdditionalClasses: function () {
Copy link
Member

Choose a reason for hiding this comment

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

toto prosim zahod a zamen za pouzitie classnames (kukni ine komponenty, napr. Loader)

export default React.createClass({
displayName: 'Icon',
propTypes: {
iconClass: React.PropTypes.string,
Copy link
Member

Choose a reason for hiding this comment

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

nemalo by byt iconClass required?

@ujovlado
Copy link
Member

ujovlado commented May 3, 2018

tie snapshoty pre Icon bude stacit pred 2-3 ikonky, len pre istotu.

@janmichek janmichek changed the title WIP: adds icons & compilation script adds icons & compilation script May 7, 2018
@janmichek
Copy link
Author

@ujovlado fixed

@ujovlado
Copy link
Member

ujovlado commented May 9, 2018

Este tie snapshoty pls #145 (comment)

@janmichek
Copy link
Author

Ok, diky. Hele ja si vubec nevim na co ty snapshoty jsou. Myslel jsem ze staci pred kazdym pushem spustit 'yarn jest -u'. Zrejme nevim k cemu to slouzi, a potrebuju se to doucit. Failuje mi to na stejny chybe jako predtim #145 (comment) -> nemuze to najit soubor s svg. Predpokladam, ze se mam ponorit do dokumentace Jestu. https://facebook.github.io/jest/

@ujovlado
Copy link
Member

ujovlado commented May 9, 2018

To neviem uplne, kde si sa docital, ze to treba pred kazdym pushom pustit. Imho je to blbost, potom by tie snapshoty fakt nemali vyznam.

Snapshoty su na to, aby sme zarucili, ze sa pri upravach nezmeni "markup" tej komponenty - urobime jej snapshot. Ked sa nejakou zmenou stane, ze nesedi snapshot - zacne failovat test a vieme, ze sa nieco zmenilo.

To moze mat vyhody napr. vtedy ked budeme mat komponenty viac previazane a pod. A tiez napr. ked bude indigo pouzite vo viac projektoch.

T.j. cele pointa je v tom mat tie komponenty stabilne.

@ujovlado
Copy link
Member

ujovlado commented May 9, 2018

Kde ti to presne failuje? Pretoze vidim, ze posledny check presiel. https://travis-ci.org/keboola/indigo-ui/builds/375805058?utm_source=github_status&utm_medium=notification

@janmichek
Copy link
Author

Pridal jsem test pro Icon.
Ten test samozrejme neprochazi, protoze jsem nevytvoril snapshot.
Snapshot mi nejde vytvorit. hazi mi to tuhle chybu. Vypada to ze tomu testu se nelibi, ze zcatek svg souboru zacina znakem '<'. Vubec nevim, zkusim to progooglit

FAIL src\indigo\components\Icon.test.js
● Test suite failed to run

D:\DEV\KEBOOLA\indigo-ui\src\indigo\img\symbol\svg\sprite.symbol.svg:1
({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){<?xml version="1.0" encoding="

UTF-8"?><symbol viewBox="-1 -1 12 8

SyntaxError: Unexpected token <

  at ScriptTransformer._transformAndBuildScript (node_modules/jest-runtime/build/ScriptTransformer.js:289:17)
  at Object.<anonymous> (src/indigo/components/Icon.jsx:2:21)
  at Object.<anonymous> (src/indigo/components/Icon.test.js:4:13)

@janmichek
Copy link
Author

Resi s to tady, uz jsem zkousel nejaky fixy, ale jeste jsem neprojel vse jestjs/jest#2663

@ujovlado
Copy link
Member

ok, skusim na to tiez pozriet, ked je to take zapeklite ;)

@janmichek
Copy link
Author

errory z konzole fixed

@ujovlado
Copy link
Member

Tak ak to blbne "len" u teba asi by som to neriesil. Ako vidis, na Travise to prechadza.

@ujovlado
Copy link
Member

Co mi este napada, ak mas Docker, ci to neskusis tak.

@ujovlado
Copy link
Member

Inac to tu asi este raz skontrolujem a myslim, ze je to ready na merge.

@janmichek
Copy link
Author

ahaak, dobrej tip. Navzdory ocekavani, mi ten test spusteny v dockeru neprojde ./ Myslim ze mi zatim bude stacit spousteni testu na Travisu, jako workaround.

@ujovlado
Copy link
Member

tvl, skoda, ze to nepomohlo ... ak mi to napadne, skusim to ked budem nieco riesit na windowsoch

@ujovlado ujovlado merged commit 6d0a99f into master May 15, 2018
@ujovlado ujovlado deleted the jan-svg-icons branch May 15, 2018 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants