-
Notifications
You must be signed in to change notification settings - Fork 24
Conversation
✔️ Deploy Preview for silacak ready! 🔨 Explore the source changes: 906c3c5 🔍 Inspect the deploy log: https://app.netlify.com/sites/silacak/deploys/611151b8390ac100070c4c97 😎 Browse the preview: https://deploy-preview-18--silacak.netlify.app |
@@ -0,0 +1,24 @@ | |||
import Head from "next/head"; |
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.
registration.tsx
-> register.tsx
- as per the navbar.
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.
Do you think "Registration" is semantically better as the navigation word is "Registrasi"?
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.
@adityapurwa Hmm, you're right. Feel free to pick whichever you like, but please update the navbar items accordingly.
import { ContentBoard } from "~/components/ui/board/content-board/content-board"; | ||
import { FormTitle } from "~/components/ui/form/form-title/form-title"; | ||
import { FormSubtitle } from "~/components/ui/form/form-subtitle/form-subtitle"; |
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.
import { ContentBoard } from "~/components/ui/board/content-board/content-board"; | |
import { FormTitle } from "~/components/ui/form/form-title/form-title"; | |
import { FormSubtitle } from "~/components/ui/form/form-subtitle/form-subtitle"; | |
import { ContentBoard } from "~/components/ui/board/content-board"; | |
import { FormTitle } from "~/components/ui/form/form-title"; | |
import { FormSubtitle } from "~/components/ui/form/form-subtitle"; |
Can we compile these exports into each of its root folder? Having the import directory doubled /content-board/content-board
doesn't feel like good UX.
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.
Did you mean instead of having ~/components/ui/board/content-board/content-board.tsx
, we have ~/components/ui/board/content-board/index.tsx
?
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.
@adityapurwa I'm thinking we create an index.ts
file and pass all the exports through there, like this:
export * from "./content-board";
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.
I personally avoid having two ways of importing something.
By grouping and exports into an index.ts
, we'd now have two ways of importing. Directly to the component file, and using the index groupings. This would introduce inconsistencies.
It will also be problematic with the go-to source feature, as now clicking on the reference will take us to the index file instead of directly to the file of interest.
What do you think about it? Let me know if the decision stands to use export groupings. 🙌
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.
@adityapurwa If you think the content-board
folder only contains this one file, I think it's okay to move the content-board.tsx
file one folder up (~/components/ui/board/content-board.tsx
).
The export grouping I think is only necessary when the folder contains a lot of components. Since export grouping is already used extensively at WBW, and we want to keep our styleguide as close to it as possible, we'll eventually have to use export grouping when we want to group exports of design system components (e.g. those at ~/components/ui/*
).
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.
Yes you are correct, I agree we can lift the component file outside of its folder. Even when we adds *.test later, I don't think it'd be too crowded. Let's go with uplifting it then 🚀
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.
Even when we adds *.test later, I don't think it'd be too crowded.
We store tests in a separate __tests__
folder so it wouldn't be a problem regardless. See Jest config for the regex pattern.
@adityapurwa Think HomePageContent, but with a background of |
How can i contribute ? |
Can you edit the PR description? If you can, the best way to do it is to edit the PR description, add your name next to the checklist item, then create a branch based on I will start listing my name next to the checklist item as well so we don't step on each other toes. What do you think @dreamid27 ? |
The content of this form is quite long, are we aiming for a sticky header and footer layout and have the scrollbar inside the middle part? |
Edit this PR? Unfortunately i can't edit the PR..
Sure, it will be helping! |
Please comment on which checklist item that you wanted to do, and I will list your name on it. Thank you @dreamid27 ! |
Form radio button input I will take this to do, may I? |
Wonderful! Thank you so much for the help. Really appreciated! 🙌 |
I don't think that'd be a good idea. Let's stick w/ |
FYI @adityapurwa @dreamid27, you can start using our Form UI components copied from our wargabantuwarga.com project. |
Heads up ~ I will not be available this week as I will be out of the city. @dreamid27 , if you want to take over this PR, feel free to do so! |
@dreamid27, please let us know if you can still work on this. We're running short on time so we need to get this done quickly. |
Sure guys! Maybe the task will done tomorrow |
You can check the pr here yah : cc : @zainfathoni @resir014 |
Superseded by #42. |
Closes #12
Description
Current Tasks