Schedule Card Component #444
Conversation
There was a problem hiding this comment.
Ne znam koliko je bitno, ali po dizajnu kad je malo duži naziv pozicije osobe, tekst bi se treba prelomit u dva reda, a kod tebe ostane u jednom redu
kod tebe:
figma:
There was a problem hiding this comment.
To je zato sta je u figminoj komponenti sirina kartice tolika da ne moze sire. Evo provjeria san, u figmi je stavljen width na fill sta je ekvivalentno width: 100 % tako da bi sta se tice responzivnosti to i trebalo bit tako
There was a problem hiding this comment.
Lgtm, ima sitnih odstupanja u dizajnu, ali ti sam procijeni koliko su bitne pa ako misliš da jesu popravi. Osim toga, jedini concern mi je treba li razbit ScheduleCard body u manje dijelove. I zadnja sitnica, might be me, ali čini mi se ko da se kartica maalkicu presporo zatvara?
There was a problem hiding this comment.
iskreno, malo sam zbunjen koji screen u figmi trebam gledat, ali ako sam dobro upratia, po dizajnu nebi tribalo bit točkica ispod naslova u mobilnoj verziji
kod tebe:
figma:
There was a problem hiding this comment.
Bojim se da ne razumin ili nesto krivo gledas, tockice se nalaze ispod opisa u jednom i u drugom slucaju. kad postoje zahtjevi bit ce tockice i ispod njih. sta se tice dijelova komponente, vjerojatno su malo razbaruseni bas zbog dizajna prema kojem se tocno neki dijelovi moraju prikazivat/sakrit kad je kartica otvorena/zatvorena. sta se tice brzine otvaranja, prema mojoj procjeni (kako nije naglaseno u dizajnu) reka bi da je okej. ako ne valja ostavia bi mihaeli da to prokomentira, lako se namisti kasnije
There was a problem hiding this comment.
fale točkice ispod voditelja panela
kod tebe:
figma:
There was a problem hiding this comment.
ah to je kasnije dodano. sredim (jos san i provjeria s mihaelom rekla je da samo izlistam normalno pa je valjda kasnije popravila da nsian vidia)
|
|
||
| function getThemeLabel(eventTheme: Theme) { | ||
| switch (eventTheme) { | ||
| case 'dev': |
There was a problem hiding this comment.
razmisli samo ima li smisla ovdje nekakav enum napravit, ali meni je ok i ovako
There was a problem hiding this comment.
cak je odgovarajuci enum vec importan iz baze samo ga nisan koristia, good catch i popravljeno
|
|
||
| function getTypeLabel(eventType: EventType) { | ||
| switch (eventType) { | ||
| case 'lecture': |
There was a problem hiding this comment.
ako napraviš gore enum, može i ovdje
| ) as string[]; /* TODO: Prominit kada se sazna na koji je nacin ovaj podatak zapisan u bazi. Stilovi su spremni. */ | ||
| } | ||
|
|
||
| return ( |
There was a problem hiding this comment.
nisam ni ja sam pametan, ali čisto razmisli ima li možda smisla razbit neke dijelove u još manje komponente
There was a problem hiding this comment.
@dbaric99 daj ti mišljenje možda
There was a problem hiding this comment.
daj primjer sta bi izdvojia. cini mi se skoro nista ne ponavlja inace, mozda samo ovaj tag ali u nacelu je ova komponenta jedinstvena. ako mislis samo zbog strukture onda mozda i mogu ali mislim da nije toliko bitno
There was a problem hiding this comment.
Da, nisam ni mislio da će se puno toga moći reusati, zato sam i pitao Damjanu ima li ona mišljenje o tome, samo ja osobno ne volim vidjeti puno HTMLa u komponentama. Čini mi se da bi lakše bilo čitat taj kod da su npr c.speakers div, requirements section i slični dijelovi u odvojenim komponentama, ali s druge strane ima li to smisla odvajat ako se neće reusat. Vjerojatno nema smisla, samo sam predložio, ali šta se mene tiče, slobodno resolvaj comment i to je to.
https://www.figma.com/design/iRoQbImFMo9G8kSouAcL3d/DLS---DD-App?node-id=21-91&m=dev
/app/testpage and TestPage component for testing the component without a common layout