feat(design): footer component #98
Conversation
|
Someone is attempting to deploy a commit to the Cesko Digital Team on Vercel. A member of the Team first needs to authorize it. |
|
This pull request is being automatically deployed with Vercel (learn more). web-storybook – ./🔍 Inspect: https://vercel.com/ceskodigital/web-storybook/indb6bp4x web – ./🔍 Inspect: https://vercel.com/ceskodigital/web/968p2jkcm |
There was a problem hiding this comment.
Ahoj @Erbenos, vypadá to velmi dobře. Líbí se mi, jak už konečně začiná vznikat něco, na co se dá koukat. A byla radost PR procházet. 👍
Přidal jsem několik komentářů ke kódu, snad je všechno srozumitelné. Ale jinak velmi dobře odvedená práce a těším se, až to zamergujeme. 🙂
|
Cekam laptop do tydne, uz je fixly, musim jej jen dostat zpet. |
|
Až budou všechny připomínky pořešené, tak to squashnu. |
| import * as S from './styles' | ||
|
|
||
| const Footer: React.FC = () => { | ||
| const theme = useContext(ThemeContext) | ||
| const [email, setEmail] = useState('') |
There was a problem hiding this comment.
Omlouvám se, tady jsem zapomněl být více specifický jaký pattern budeme v rámci repozitáře používat. Poprosím tedy o refactorovaní, protože formulář bude do budoucna sloužit jako inspirace.
Bylo by vhodné aby stavy a logika formu byla extrahovaná do samostatného logického celku, v případě Reactu jako hook. Zkusím ukázat na příkladu:
const useForm = ({initialValues, submitForm}) => {
// .... useState
return {
values: {
email,
},
errors: {
email,
},
handlers: {
handleSubmit,
onEmailChange
}
// ...
}
}Nechávám otevřené jaké vstupní rozhraní hook bude mít (zda mít modifkovatelný onSubmit, validace atd.), ale veškerý stav pro formulář by měl být v rámci této hook. (Podobnost s Formikem není náhodná)
Nezapomenout doplnit test na validace a říkám si, jestli subscribe do newsletteru nemůže být jako REST požadavek na pozadí a ne přes HTML formulář.
There was a problem hiding this comment.
Abstrahoval jsem logiku formulářu do hooku v a5e7ba1. Abych mohl submittovat přes Ajax do ecomailu, bude nejspíše třeba API klíč (a jeho následné vložení někam do env proměnných v build procesu). https://intercom.help/ecomail/cs/articles/66536-api-pro-praci-s-ecomailem . Nemám k tomu teď přístupy. Můžeš mi je opatřit?
There was a problem hiding this comment.
API klíč nelze mít s rozsahem pouze pro subscribe, takže budeme muset klíč zakrýt přes vlastní API endpoint. Vytvořil jsem na to issue (#107) a musím promyslet, jakým způsobem to prioritizovat. Dám update během dnešního dne ještě.
Pokud jde o samotný hook, tak rozhodně krok dopředu, ale ještě tam vidím prostor pro zlepšení a je možné, že jsme se vzájemně nepochopili.
Cíl ani tak není mít obecné řešení pro formuláře (to můžeme vyřešit přidáním Formiku) jako spíše vytvoření pevně definovaného rozhraní pro formulář patičky. Jde o to, že by mělo být na první pohled jasné, jaké hodnoty jsou součástí formuláře a bezpečný způsob pro kontrolu formuláře (např. při použití Formiku se nevyužívá setFieldValue, ale logika se zanoří do handlers.emailChange: (event) => setFieldValue('email', event.target.value).
Pokud je to stále nejasné, možná bude lepší si kolem toho zavolat. Vím, že to bude další nutná změna, ale stále platí, že tímto formulářem určujeme vzor pro další, které rozhodně budou přibývat.
There was a problem hiding this comment.
Jak jsme se domluvili přes Slack/GMeet, všechno ohledně formuláře jsem purgnul 07b3d49, backup Erbenos:feat/footer-form-backup
|
Rebasnul jsem na master, kontrola prosím, squashnu to, a jsme done. |
HormCodes
force-pushed
the
feat/footer
branch
from
7159ade to
33f66b6
Compare
HormCodes
force-pushed
the
feat/footer
branch
from
33f66b6 to
cf30323
Compare
Fixes #75
Nevím jestli je třeba Storybook i pro komponenty bez parametrů, tak případně někdo prosím pípněte.
Musel jsem trochu porušit enkapsulaci odkazů, protože nemáme bílé Link-y.
Laptop mám teď v servisu 😭, tak nebudu moc být tak flexibilní s adresací případných problému, jak bych chtěl.