-
Notifications
You must be signed in to change notification settings - Fork 39
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
Kolonka "Prostředí" může nabývat vícero hodnot #1128
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super, díky moc!
Bádám akorát nad tím, jak to ukládat v databázi, tedy jestli nepřejít z reprezentace řetězcem na regulérní multi select. Tím pádem bychom měli i v modelu na klientovi ReadonlyArray<string>
namísto string
. Na straně té databáze se s tím líp pracuje, ale zase je to trochu „tužší“, striktnější model, se kterým se pak musíme víc drbat při změnách. Máš na to nějaké názory? Díky!
@@ -19,24 +19,25 @@ export const OccupationSelect = ({ | |||
"looking-for-job": "Hledám práci", | |||
"other": "Jiné", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Navrhuju ještě v rámci téhle změny zrušit možnost Jiné, s těmi checkboxy už mně definitivně nedává smysl :) V databázi projdu a zahodím.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smáznu.
@@ -45,3 +46,15 @@ export const OccupationSelect = ({ | |||
</div> | |||
); | |||
}; | |||
|
|||
const encodeSelection = (occupation: Set<string>, id: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chvilku jsem tady bádal nad tím, jestli nemít samostatně něco jako flip<T>(values: Set<T>, value: T)
a samostatně encode(values: Set<string>)
, ale je to samozřejmě naprostá podružnost :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jakože 2 obecný funkce?
flip
by buď odebrala z množiny nebo přidala do množiny prvek.encode
by převáděla množinu na string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Přesně tak. Když pak budu číst encodeSelection(flip(occupationSet, id))
, je mně to jasnější.
Nad tim jsem taky přemejšlel a původně jsem to tak chtěl udělat. Přišlo mi pak ale, že je to celkem dost velkej zásah na to, jak marginální změna to je. Takže jsem se inspiroval těmi ostatními multi selecty a vyhrála u mě spíš ta konzistence (přišlo mi blbý mít to jednou tak a po druhý jinak). Rozhodně to ale můžeme změnit, z pohledu nějakýho návrhu je to určitě rozumnější schéma, jen by to pak chtělo změnit i na ostatních polích teda. Plus bych případně potřeboval poradit, jak s DB migrací. Ničeho takovýho jsem si v projektu nevšiml a možná to děláš jen ručně? 😅 |
Je to tak, žiju nebezpečně a migruju ručně :) Obávám se, že to v té Airtable ani jinak nejde. Museli bychom asi založit nový sloupec, skriptem do něj překlopit ta data a následně smazat původní sloupec a přejmenovat nový. Plus upravit kód na klientovi. Pojďme to teď klidně přeskočit :) Můžem udělat kdykoliv pozděj a případně při té příležitosti popřemýšlet, jestli takhle nepřevést i další pole. Mimochodem ještě mě napadlo, že jsme tady asi neaktualizovali zobrazení toho pole v uživatelském profilu. Tam se počítá jen s jednou hodnotou, bude potřeba tam přidat podporu více hodnot. (To je nevýhoda těch „netypovaných řetězců“ :) |
OK OK, kašlem na to prozatím. 🙂
Good catch! To mi nedošlo, ale opravim. 👍 |
Closes #1111