Feat/homepage#20
Conversation
MeesMaas
left a comment
MeesMaas
left a comment
There was a problem hiding this comment.
Ik kan de review helaas niet afmaken wegens tijdgebrek maar hier alvast wat comments
| <div class="flex gap-5"> | ||
| {/* Activity Enrollments */} | ||
| <Tile class="bg-(--theme-460) border-2 border-white/20 grow"> | ||
| <p>Aanmeldingen</p> |
There was a problem hiding this comment.
Willen we meteen ook iets als i18n er in fietsen voor vertalingen?
There was a problem hiding this comment.
Had het er nu nog uitgelaten maar is wel iets wat snel moet idd, kunnen we of nu meepakken of als volgende feature doen
There was a problem hiding this comment.
Toch al toegevoegd :)
| }, | ||
|
|
||
| setup(props) { | ||
| // Path to sticky logo |
There was a problem hiding this comment.
Moet dit niet in een global constant oid?
There was a problem hiding this comment.
Ik vermoed dat we t sticky logo voor de rest nergens in koala nodig gaan hebben
There was a problem hiding this comment.
Tot het wel zo is. Daarom is dit perfect om in een const genaamd images te gooien die in de lib folder komt. Dit is gewoon vervelend als het logo ook ooit op een andere plek komt te staan. dan moet je gaan zoeken in de code.
export default {
sticky_logo: '..'
};
| class={` | ||
| flex items-center gap-2 rounded-xl border-2 border-transparent | ||
| py-1 px-2 | ||
| ${ | ||
| props.isMobile | ||
| ? "w-full justify-start py-2 px-3" | ||
| : "cursor-pointer transition-colors duration-200 ease-in-out hover:bg-(--theme-460) hover:border-white/20" | ||
| } | ||
| `} |
There was a problem hiding this comment.
waarom isMobile? tailwind heeft toch al die breakpoints van sm:w-full en md:w-25 bijvoorbeeld?
There was a problem hiding this comment.
Klopt, maar dan kan je t niet in de code gebruiken om bijvoorbeeld uit te zetten dat er wat gebeurt als je klikt op de profile name/picture
Co-authored-by: Mees Maas <MeesMaas@users.noreply.github.com>
…to feat/homepage
MeesMaas
left a comment
MeesMaas
left a comment
There was a problem hiding this comment.
Okay ik heb de review nu wel af kunnen maken
| import type { Announcement } from "@/Types/Announcement"; | ||
|
|
||
| export default defineComponent({ | ||
| name: "AnnouncementsList", |
There was a problem hiding this comment.
Je bestandsnaam heet AnnouncementList en hier schrijf je het met een s erbij
| setup(props) { | ||
| return () => ( | ||
| <ListTile class="w-full"> | ||
| {props.CommitteeEnrollments.map((CommitteeEnrollment) => ( |
There was a problem hiding this comment.
Hoe gaat dit component werken als je al meerdere jaren in een committee zit? Is daar rekening mee gehouden of komt dat in een volgende Pr?
There was a problem hiding this comment.
Moeten we nog naar kijken hoe we dat willen, maar ik dacht dat dat iets was wat we met de verbinding met backend moeten doen (en niet in het component zelf)
| const handleClick = () => { | ||
| props.onClick?.(); | ||
| }; |
There was a problem hiding this comment.
Een hele functie die maar 1x wordt gebruikt lijkt mij persoonlijk overkill
There was a problem hiding this comment.
Als ik m niet als functie doe krijg ik dit:
Type 'void | undefined' is not assignable to type '((payload: PointerEvent) => void) | undefined'. Type 'void' is not assignable to type '((payload: PointerEvent) => void) | undefined'.
ik ga op zoek naar een andere oplossing
There was a problem hiding this comment.
wait ik ben dom, gefixt
| // Function to update visible activities based on container width | ||
| const updateVisible = () => { | ||
| if (!containerRef.value) return; | ||
| const containerWidth = containerRef.value.getBoundingClientRect().width; | ||
| const tileWidth = 200; | ||
| const possibleTileBesidesEachother = Math.floor( | ||
| containerWidth / tileWidth, | ||
| ); | ||
| comingActivitiesBelowEachother.value = possibleTileBesidesEachother === 1; | ||
| const count = comingActivitiesBelowEachother.value | ||
| ? 3 | ||
| : possibleTileBesidesEachother; | ||
| visibleActivities.value = props.activities.slice(0, count); | ||
| }; | ||
|
|
||
| // Set up event listeners | ||
| onMounted(() => { | ||
| nextTick(updateVisible); | ||
| window.addEventListener("resize", updateVisible); | ||
| }); | ||
|
|
||
| // Clean up event listeners | ||
| onBeforeUnmount(() => { | ||
| window.removeEventListener("resize", updateVisible); | ||
| }); |
There was a problem hiding this comment.
Deze logica voelt een beetje overbodig. Ik heb liever dat je dan kijkt naar grid md:grid-cols-3
There was a problem hiding this comment.
Probleem waar ik mee zat was dat ik het zonder de logica niet voor elkaar kreeg om zoveel items als past op een regel te krijgen zonder dat er ook items op de volgende regel komen
There was a problem hiding this comment.
in principe kan je utils over meerdere bestanden uitspreiden. Dus dates.utils.ts en idk what die andere util functie is. Maar vnode.utils.ts zodat je echt structuur krijgt. Anders gaat dit bestand enorm uitgroeien.
| return () => { | ||
| // Flatten the slot children | ||
| const raw = slots.default?.() ?? []; | ||
| const children = flattenChildren(raw); |
There was a problem hiding this comment.
Volgens mij kan dit trucje ook zonder dat je flattenChildren hoeft te doen. Dit doe je puur om de divider er tussen te krijgen toch?
There was a problem hiding this comment.
ik had t eerst zonder de flattenChildren maar toen splitste die niet goed de divs erin uit
2 participants
Home page and menu bar