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

[WIP] Add login screen #8

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

renekliment
Copy link

@renekliment renekliment commented Aug 14, 2023

This PR fixes #7 once merged.

Preview

Aplikace je automaticky deployovaná pomocí Cloudflare Pages na https://rk-task-mild-blue.pages.dev/.

TODOs

Otázky na zadání v issue

  • ujasnit zamýšlenou podobu state/store a useUser hooku
  • jak to má být s tím "aplikace vyhodí Error"?
  • vyřešit ostatní drobné dotazy

Hlavní

  • doladit CSS, aby důkladněji odpovídalo předloze

Disclaimers & design decisions

  • V reálné aplikaci bych pro styly použil spíše SASS/LESS, či Tailwind, ale tady mi to přišlo trochu overkill. Hlavní funkcionalitu co jsem potřeboval jsem pokryl CSS Variables, které jsou dobře podporované. SASS, či LESS by mi tady ušetřili opakování názvů komponent když bych použil nesting, ale to mi nepřišlo v této malé aplikaci tolik užitečné. Použil jsem tedy obyčejné CSS s BEMem (resp. mírně modifikovanou syntaxí, ale stejným sentimentem).
  • Pro šipky v tlačítku a u odkazu jsem místo (SVG) obrázku použil obyčejný UTF znak. Sice to pak asi nebude vypadat úplně stejně na všech platformách, ale připadá mi to jako správnější, jednodušší řešení. Možná to bude vypadat vzhledem k textu lépe/konzistentně v rámci platforem ... možná ne.
  • V reálné aplikaci bych pro stav/store použil redux-toolkit, či alespoň useReducer hook (ten případně na vyžádání můžu použít, to by nezabralo moc času), ale nakonec jsem pro tuto demo aplikaci zvolil obyčejný useState s useContent (moje první použití custom contextu, tak snad je idiomatické), protože jsem to nechtěl overengineerovat.
  • Mám problém s rozpoznáním barev (barvoslepost?), takže snad jsem nepřehlédl očividné barvy, které mají někde být jinak. Kopíroval jsem vše z Adobe věci, tak snad jsem nic nepřehlédl.
  • Toto je mé první použití testing-library, tak snad je idiomatické. V předchozí práci jsme používali React 16 & enzyme, kdy paradigma testování s enzymem bylo spíš testování implementace. Sentiment testing-library s testováním co nejvíce jako uživatel znám, jen v něm nemám takovou praxi, tak snad to vypadá rozumně.
  • Testy jsou možná trochu overkill, kdy se testuje flow všech kombinací s počáteční vybranou hodnotou, vybráním a očekávanou. Nevím, jestli bych to takhle udělal i v reálné aplikaci, ale meh.
  • Commity jsou rozdělené spíše podle komponent/souborů než "stavíme dům po patrech", což dělám běžněji, ale tady to nedopadlo, zpětně by byl moc effort to předělat, tak nechávám takto.

Further

Kdyby bylo cílem věnovat tomu více effortu, udělal bych asi následující:

  • Všiml jsem si, že při přeskakování z jedné screeny na druhou se pak možná nechová tlačítko "zpět" ideálně/očekávaně. Možná by bylo dobré, aby se do browser history uchovávala každá z těch screen jen jednou. Jak by se to skutečně mělo chovat je asi dost na diskusi, tak to nechávám defaultně.
  • Aktualizace README, aby vypovídalo o tom, co aplikace dělá.
  • Přidání react-helmet, aby byly smysluplné titulky.
  • Přidání Dockerfile, aby se to dalo stabilně buildit a nasazovat.

Copy link
Collaborator

@misslecter misslecter left a comment

Choose a reason for hiding this comment

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

Hezká práce!

Hodně ceníme pár věcí co jsi zapracoval:

  • testy
  • pokročilé využití react hooků
  • Omit 👍

Vyčetla bych ti akorát vizuální stránku, která nevypadá úplně podle designu, což je třeba box nezarovnany doprostřed a neprepsany základní styly pro element (minimálně v Google Chrome tlačítko má defaultní ohraničení).

Taky v našem stacku nepoužíváme moc CSS, ale ceníme to, že jsi se s nimi dokázal poradit.

Každopádně za mě je to okay.
Každopádně dobrá práce,

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.

Implement login screen design
2 participants