search

keboola / indigo-ui

Indigo UI library, its CSS and React components. Also Styleguide app showing usages of this library.
https://indigo.keboola.com
MIT License
5 stars 0 forks source link

Finished component migration #315

Closed janmichek closed 5 years ago

janmichek commented 5 years ago

Jeste to potrebuju projit a vyscreenovat

natocTo commented 5 years ago

Jen jsem do toho nahlédnul, mohlo by se vyžadovat nejnovější moment + moment-timezone. V kbc-ui to také takto je https://github.com/keboola/kbc-ui/pull/2303

janmichek commented 5 years ago

jj, vim ze jsi to nedavno upgradoval, chtel jsem ty verze jeste zvalidovat. Dik

ujovlado commented 5 years ago

Tam potom stoji za uvazenie, ci by nemalo ist o peerDependencies

janmichek commented 5 years ago

Nakonec jsem odebral custom datetime format z props - zkratka tak jak to bylo puvodne. Je to snadno rozsiritelny.

natocTo commented 5 years ago

Drobnost :) moment-timezone nikde nevidím použitý, tak možná tu nebude potřeba. Pokud jo, tak bych to hodil přímo pod "dependencies" stejně jako moment

natocTo commented 5 years ago

Ještě koukám že endTime má být string, ale v demu (stories) je tam posílán int. Nevím jak v KBC, aby tam sěděl ten typ props.

natocTo commented 5 years ago

Myslím že by stálo za to přidat novou CSS třídu icon-with-label nebo fa-with-label nebo obecně mr-5 třeba. Která by nastavila "margin-right: 5px" třeba.

Mělo by to kupu využití a nemusela by se tolik používat {' '} respektive obalovat něco spanem jen abych přidal mezeru. Co si o tom myslíte? :)

janmichek commented 5 years ago

diky za review

  1. moment-timezone - revertnu
  2. endTime format - fixnu to a zdokumentuju v Indigu. Ted jsem s tim zapasil pri dalsi komponente
  3. vyresim tu mezeru pomoci stylu. Jinak jsem pro proprietarni tridy. I kdyz je to nesystematicky reseni, tak to usetri spoustu radku kodu, coz prevazi ve vyhodu. Musi ale progamator vedet co dela, muze se to snadno zvrhnout xD
janmichek commented 5 years ago

na ten format props se jeste musim podivat, nejaky divny

ujovlado commented 5 years ago

Len poznamka k tomu {' '}. Ma to svoje vyuzitie. Dolezite je si uvedomit, ze sirka medzery je variabilna vzhladom na velkost fontu. Ak sa obmedzime na margin-right: 5px, tak to bude stale 5px a je jedno ci to pouzijem v h1 alebo small. Medzera sa tak nesprava (co je dobre) a ma vzdy sirku vzhladom na velkost pisma.

Pokial viem, tak "universal margin/padding" classes (ako su napr. v Bootstrap 4) si mi @janmichek zmietol zo stola - ze to bude bordel. :) https://github.com/keboola/indigo-ui/issues/106

natocTo commented 5 years ago

Místo 'px' se může použít 'rem' to by mělo být teda lepší.

Rem se používá třeba právě v utilities bootsrapu 4 pro margin, padding.

ujovlado commented 5 years ago

Asi by to slo (podobne som to mal v #107). Neriesil by som to ale v tomto PR.

janmichek commented 5 years ago

U toho testu jeste potrebuju vyresit zavislost na case - protoze komponenta pak hazi jiny snapshoty v zavislosti na case spusteni testu.

natocTo commented 5 years ago

Když si otevřu v chrome dostávám tuto hlášku:

Deprecation warning: value provided is not in a recognized RFC2822 or ISO format. moment construction falls back to js Date(), which is not reliable across all browsers and versions. Non RFC2822/ISO date formats are discouraged and will be removed in an upcoming major release. Please refer to http://momentjs.com/guides/#/warnings/js-date/ for more info. Arguments:

Je to tím jak tam jde string místo int. Pokud má ta komponenta podporovat různé formáty jak tam asi musí být nějaká kontrla zda je o unix time string a případně upravit.

Ještě bych zkusil zda nebude teda lepší využívat "rem" místo "px". A ještě prověřuji něco tak to sepíši jen co nainstaluji node-modules :)

natocTo commented 5 years ago

Podle mě je otázka zda moment by měl být peerDependencies. Pak teda všude kde bude indigo-ui, bude muset být instalován moment. Což jsem myslel že nemusí být pravda. U Reactu to je jasný, když je to react ui library. Ale u toho momentu nevím.

Jinak pokud tam bude tak by mělo být taky jako devDependencies. Teď se do indiga dostane jen díky tomu že to vyžaduje jiná knihovna nebo tak. Ale to tak nemusí být vždy.

Tak bych to klidně dal něco jako:

"devDependencies": {
  "moment": "^2.22.2"
}

"peerDependencies": {
  "moment": "^2.18.0"
}

Použít nejnovější verzi. Ale pro jiné knihovny řici že musí mít alespoň tu 2.18 na kterém KBC-UI také jelo v pohodě.

ujovlado commented 5 years ago

No chcel som o peerDependencies trosku diskutovat, preto som to spomenul.

Pri tom Reacte to ma vyznam a je to jasne - aby sme nedostali 2 instalacie Reactu, co moze robit bordel.

Pri Momente mi skor ide o to, aby sa nam nestalo, ze nejaky nedopatrenim budeme mat v bundle 2x moment, ci sa zvysi bundle size.

natocTo commented 5 years ago

Jasné. Ale teda mělo by to být také v devDependencies. Jinak podobně by mohlo být pro immutable jako devDependencies + peer. Ale to je asi zase jiná diskuze.

ujovlado commented 5 years ago

Ano, to som mozno nenapisal jasne. Pre vyvoj v dev, pre disttribuovanie v peer ... Robil som to tak napr. pre React alebo React Boostrap.

janmichek commented 5 years ago

upravil jsem ty dependencies, podle navrhu + fixnul ten datetime format. Cpal jsem tam jinej nez v produkci

natocTo commented 5 years ago

Ještě nevím co ty jednotky. Možné se řeší v jiném vláknu. Myslím to "px" vs "rem". Že vůči font-size bude asi lepší používat "rem".

Ještě koukám na ty testy. Ono to je asi vždy trochu problematické testovat takto s časem apod. Moc s tím zkušenosti nemám ale aspoň si to projdu ještě.

natocTo commented 5 years ago

Nevím zda to je lepší nebo horší ale mě u testů zafunguje i toto. Jako endTime props posílám jen string místo Date. Myslím že to tak je v KBC-UI kde chodí string s api.

const DATE_TO_USE = '2016-01-01T00:00:00+0100';

global.Date.now = jest.fn(() => new Date(DATE_TO_USE).valueOf());

describe('<Finished />', () => {
  it('Basic Init', () => {
    snapshot(<Finished endTime={DATE_TO_USE} />);
  });
});
janmichek commented 5 years ago

"px" vs "rem" Dobre, muzu to upravit v tech poslednich 2 komponentach. Zatim je to asi jedno, ale do budoucna se s tim asi da pocitat, ze se to pouzije na vice mistech s ruznou velikosti pisma

janmichek commented 5 years ago

ta 'rem' jednotka nic nedela - margin ikony se zvetsi jen pokud je na <html> naveseny font-size v px. Kdyz bude cela aplikace takhle relativne nastavena, tak potom to bude fungovat.

natocTo commented 5 years ago

Jej, tak co 'em' :) Já mám v tomto vždy guláš. REM bude jako Root EM, takže asi to HTML. A EM by teda mohlo byt podle toho co je nad.

janmichek commented 5 years ago

ok, upravil jsem na 'em'

image