Challenge tabs #90
Challenge tabs #90
Conversation
ssynowiec
commented
Mar 9, 2023
•
ssynowiec
commented
apps/web/components/organisms/newOpinionForm/newOpinionForm.tsx
Outdated
Show resolved
Hide resolved
apps/web/components/organisms/protectedPage/protectedComponent/protectedComponent.tsx
Outdated
Show resolved
Hide resolved
purple review Co-authored-by: prplblck <[email protected]>
apps/web/components/organisms/newOpinionForm/newOpinionForm.tsx
Outdated
Show resolved
Hide resolved
apps/web/components/molecules/commentItem/commentItemWithReply.tsx
Outdated
Show resolved
Hide resolved
| <div className="flex-1 px-2 ml-2 text-sm font-medium leading-loose text-gray-600"> | ||
| {comment} | ||
| </div> | ||
| <button className="inline-flex items-center px-1 pt-2 ml-1 flex-column"> |
There was a problem hiding this comment.
This 2 Reply and Like buttons share styles. Same with the div inside. I'd make a component out of this.
The only difference is the pt-2 which can be applied on some container or spacer.
apps/web/components/molecules/commentItem/commentItemWithReply.tsx
Outdated
Show resolved
Hide resolved
apps/web/components/molecules/commentItem/commentItemWithReply.tsx
Outdated
Show resolved
Hide resolved
apps/web/components/organisms/newOpinionForm/newOpinionForm.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| type ProtectedComponentProps = { | ||
| info?: string; | ||
| children: ReactNode; |
There was a problem hiding this comment.
Maybe adding an optional slot for a component that should be displayed in case user is not logged in?
| @@ -66,6 +67,9 @@ export const SingleChallengePage = ({ | |||
| </div> | |||
| </div> | |||
| </section> | |||
| <section> | |||
| <Tabs /> | |||
There was a problem hiding this comment.
Maybe something like DiscussionTabs or make it some discussion section?
apps/web/types/types.tsx
Outdated
| author: string; | ||
| avatar: string; | ||
| comment: string; | ||
| date: number; |
There was a problem hiding this comment.
Is it UTC timestamp? Are you thinking about specific users from specific timezone? (target)
| </button> | ||
| <button className="inline-flex items-center px-1 -ml-1 flex-column"> | ||
| <div className="w-5 h-5"> | ||
| <Liked /> |
There was a problem hiding this comment.
This buttons with icons are a good candidate for a separate component as well ;)
|
|
||
| type CommentItemProps = Comment; | ||
|
|
||
| export const CommentItem = ({ | ||
| author, | ||
| avatar, | ||
| comment, | ||
| date, | ||
| timestamp, | ||
| }: CommentItemProps) => { |
There was a problem hiding this comment.
I'd make a comment: Comment prop accepting an object. No need for spreading then and you have everything contained in one object
There was a problem hiding this comment.
I'd make a
comment: Commentprop accepting an object. No need for spreading then and you have everything contained in one object
Sure, but you neeed to destructure it later or write redundant object prop calls
comment.timestamp
comment.avatar
...and so on
There was a problem hiding this comment.
I'd rather have a param destructurization than something like that tbh.
As for now the CommentItemProps is redundant as the props could be directly typed as Comment.
I used to spread props but over the time I learned in most of the cases it is better to explicitly pass props. There are some exceptions ofc (for example register function for form libs)
| const sizes = { | ||
| 24: 'w-24 h-24', | ||
| 12: 'w-12 h-12', | ||
| 10: 'w-10 h-10', | ||
| }; |
There was a problem hiding this comment.
This can be placed outside of the component (above component declaration) as a const
SIZES or AVATAR_SIZES
