Open MrBisquit opened 12 months ago
Was there a particular reason for using inline styles instead of Tailwind in App.tsx
?
Was there a particular reason for using inline styles instead of Tailwind in
App.tsx
?
Not really, but just seemed sensible because the buttons at the top were using it, also theres no point in making it look nice because it'll be removed later anyway.
There's a lot of duplicate code within the Alert components across multiple files
There's a lot of duplicate code within the Alert components across multiple files
There's no duplicate code
There's a lot of duplicate code within the Alert components across multiple files
There's no duplicate code
For example
export default function DeleteRoom({ children }) {
return (
<AlertDialog>
<AlertDialogTrigger>{children}</AlertDialogTrigger>
<AlertDialogContent>
<AlertDialogHeader>
<AlertDialogTitle style={{ color: "#2AC3DE" }}>Are you sure?</AlertDialogTitle>
<AlertDialogDescription>
This will delete the room and kick everyone from it.
</AlertDialogDescription>
</AlertDialogHeader>
<AlertDialogFooter>
<AlertDialogCancel>Cancel</AlertDialogCancel>
<AlertDialogAction>Continue</AlertDialogAction>
</AlertDialogFooter>
</AlertDialogContent>
</AlertDialog>
)
}
This could be turned into a reusable component
There's a lot of duplicate code within the Alert components across multiple files
There's no duplicate code
For example
export default function DeleteRoom({ children }) { return ( <AlertDialog> <AlertDialogTrigger>{children}</AlertDialogTrigger> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle style={{ color: "#2AC3DE" }}>Are you sure?</AlertDialogTitle> <AlertDialogDescription> This will delete the room and kick everyone from it. </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> <AlertDialogCancel>Cancel</AlertDialogCancel> <AlertDialogAction>Continue</AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> </AlertDialog> ) }
This for example couple be a reusable component
Not really, The Alert Dialog components are composable UI components from Shadcn UI that's used to build the components specific to our domain. DeleteRoom is reusable which is what we need. Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
There's a lot of duplicate code within the Alert components across multiple files
There's no duplicate code
For example
export default function DeleteRoom({ children }) { return ( <AlertDialog> <AlertDialogTrigger>{children}</AlertDialogTrigger> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle style={{ color: "#2AC3DE" }}>Are you sure?</AlertDialogTitle> <AlertDialogDescription> This will delete the room and kick everyone from it. </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> <AlertDialogCancel>Cancel</AlertDialogCancel> <AlertDialogAction>Continue</AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> </AlertDialog> ) }
This for example couple be a reusable component
Not really, The Alert Dialog components are composable UI components from Shadcn UI that's used to build the components specific to our domain. DeleteRoom is reusable which is what we need. Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
This is what I'm talking about. The code between deleteProject.tsx and deleteRoom.tsx
There's a lot of duplicate code within the Alert components across multiple files
There's no duplicate code
For example
export default function DeleteRoom({ children }) { return ( <AlertDialog> <AlertDialogTrigger>{children}</AlertDialogTrigger> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle style={{ color: "#2AC3DE" }}>Are you sure?</AlertDialogTitle> <AlertDialogDescription> This will delete the room and kick everyone from it. </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> <AlertDialogCancel>Cancel</AlertDialogCancel> <AlertDialogAction>Continue</AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> </AlertDialog> ) }
This for example couple be a reusable component
Not really, The Alert Dialog components are composable UI components from Shadcn UI that's used to build the components specific to our domain. DeleteRoom is reusable which is what we need. Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
This is what I'm talking about. The code between deleteProject.tsx and deleteRoom.tsx
I don't think we have to do that. DRY is a a best practice but should be used wisely. if we have a dynamic component for example DestructiveAction
and then give it props for the components of the dialog, sure that'd be reusable but that might be premature abstraction because we may not have a lot of destructive actions. Readability may also be affected as the Dev would have to figure out what type of action the component is for versus if the component name is exactly what it does DeleteRoom
. I'd say this is not a big deal and personally think the code's fine as it is right now.
Are the component props missing types?
The components look pretty duplicated, much of the duplication can be solved by passing props into the components, title, description, action buttons etc
You only really need one version of the Alert Dialog as the formatting is very similar
You could also move the long text out in to a lang file to clean the logic up (but very optional)
The components look pretty duplicated, much of the duplication can be solved by passing props into the components, title, description, action buttons etc
You only really need one version of the Alert Dialog as the formatting is very similar
You could also move the long text out in to a lang file to clean the logic up (but very optional)
As mentioned earlier on, that's not really necessary right at this moment. I already tried to make multiple languages using lang files but there's no real point.
The components look pretty duplicated, much of the duplication can be solved by passing props into the components, title, description, action buttons etc You only really need one version of the Alert Dialog as the formatting is very similar You could also move the long text out in to a lang file to clean the logic up (but very optional)
As mentioned earlier on, that's not really necessary right at this moment. I already tried to make multiple languages using lang files but there's no real point.
Lack of necessity is never an excuse for ignoring good design principles, theres no rush or urgent deadline, so we should get it right first time
There's a lot of duplicate code within the Alert components across multiple files
There's no duplicate code
For example
export default function DeleteRoom({ children }) { return ( <AlertDialog> <AlertDialogTrigger>{children}</AlertDialogTrigger> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle style={{ color: "#2AC3DE" }}>Are you sure?</AlertDialogTitle> <AlertDialogDescription> This will delete the room and kick everyone from it. </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> <AlertDialogCancel>Cancel</AlertDialogCancel> <AlertDialogAction>Continue</AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> </AlertDialog> ) }
This for example couple be a reusable component
Not really, The Alert Dialog components are composable UI components from Shadcn UI that's used to build the components specific to our domain. DeleteRoom is reusable which is what we need. Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
This is what I'm talking about. The code between deleteProject.tsx and deleteRoom.tsxI don't think we have to do that. DRY is a a best practice but should be used wisely. if we have a dynamic component for example
DestructiveAction
and then give it props for the components of the dialog, sure that'd be reusable but that might be premature abstraction because we may not have a lot of destructive actions. Readability may also be affected as the Dev would have to figure out what type of action the component is for versus if the component name is exactly what it doesDeleteRoom
. I'd say this is not a big deal and personally think the code's fine as it is right now.
Sort of, I would at least rip out the layout into a base Alert component that accepts some props. Then you could still have the deleteRoom.tsx components etc if you needed reuse. No need to make it too complicated with logic. The main thing is keeping it DRY.
import BaseAlert from './BaseAlert';
type DeleteRoomProps = {
children: React.ReactNode;
};
const DeleteRoom = ( { children }: DeleteRoomProps ) => (
<BaseAlert
title='Are you sure?'
description='This will delete the room and kick everyone from it.'
cancelButtonText='Cancel'
actionButtonText='Continue'
>
{children}
</BaseAlert>
);
export default DeleteRoom;
Something like that
I would also like to add to the PR in general, it's a good idea to make smaller PR's so they're easier to review, smaller commits and better naming them. Break things down into bite sized chunks.
There's a lot of duplicate code within the Alert components across multiple files
There's no duplicate code
For example
export default function DeleteRoom({ children }) { return ( <AlertDialog> <AlertDialogTrigger>{children}</AlertDialogTrigger> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle style={{ color: "#2AC3DE" }}>Are you sure?</AlertDialogTitle> <AlertDialogDescription> This will delete the room and kick everyone from it. </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> <AlertDialogCancel>Cancel</AlertDialogCancel> <AlertDialogAction>Continue</AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> </AlertDialog> ) }
This for example couple be a reusable component
Not really, The Alert Dialog components are composable UI components from Shadcn UI that's used to build the components specific to our domain. DeleteRoom is reusable which is what we need. Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
This is what I'm talking about. The code between deleteProject.tsx and deleteRoom.tsxI don't think we have to do that. DRY is a a best practice but should be used wisely. if we have a dynamic component for example
DestructiveAction
and then give it props for the components of the dialog, sure that'd be reusable but that might be premature abstraction because we may not have a lot of destructive actions. Readability may also be affected as the Dev would have to figure out what type of action the component is for versus if the component name is exactly what it doesDeleteRoom
. I'd say this is not a big deal and personally think the code's fine as it is right now.Sort of, I would at least rip out the layout into a base Alert component that accepts some props. Then you could still have the deleteRoom.tsx components etc if you needed reuse. No need to make it too complicated with logic. The main thing is keeping it DRY.
import BaseAlert from './BaseAlert'; type DeleteRoomProps = { children: React.ReactNode; }; const DeleteRoom = ( { children }: DeleteRoomProps ) => ( <BaseAlert title='Are you sure?' description='This will delete the room and kick everyone from it.' cancelButtonText='Cancel' actionButtonText='Continue' > {children} </BaseAlert> ); export default DeleteRoom;
Something like that
what's the point of children? Where would children go into in Nevermind, missed the Trigger componentBaseAlert
?
There's a lot of duplicate code within the Alert components across multiple files
There's no duplicate code
For example
export default function DeleteRoom({ children }) { return ( <AlertDialog> <AlertDialogTrigger>{children}</AlertDialogTrigger> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle style={{ color: "#2AC3DE" }}>Are you sure?</AlertDialogTitle> <AlertDialogDescription> This will delete the room and kick everyone from it. </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> <AlertDialogCancel>Cancel</AlertDialogCancel> <AlertDialogAction>Continue</AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> </AlertDialog> ) }
This for example couple be a reusable component
Not really, The Alert Dialog components are composable UI components from Shadcn UI that's used to build the components specific to our domain. DeleteRoom is reusable which is what we need. Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
This is what I'm talking about. The code between deleteProject.tsx and deleteRoom.tsxI don't think we have to do that. DRY is a a best practice but should be used wisely. if we have a dynamic component for example
DestructiveAction
and then give it props for the components of the dialog, sure that'd be reusable but that might be premature abstraction because we may not have a lot of destructive actions. Readability may also be affected as the Dev would have to figure out what type of action the component is for versus if the component name is exactly what it doesDeleteRoom
. I'd say this is not a big deal and personally think the code's fine as it is right now.Sort of, I would at least rip out the layout into a base Alert component that accepts some props. Then you could still have the deleteRoom.tsx components etc if you needed reuse. No need to make it too complicated with logic. The main thing is keeping it DRY.
import BaseAlert from './BaseAlert'; type DeleteRoomProps = { children: React.ReactNode; }; const DeleteRoom = ( { children }: DeleteRoomProps ) => ( <BaseAlert title='Are you sure?' description='This will delete the room and kick everyone from it.' cancelButtonText='Cancel' actionButtonText='Continue' > {children} </BaseAlert> ); export default DeleteRoom;
Something like that
what's the point of children? Where would children go into in
BaseAlert
?
Check out the code in the PR. children
is being used for a lot. I'm not questioning it though :) It's also referenced in this quote chain at the top
There's a lot of duplicate code within the Alert components across multiple files
There's no duplicate code
For example
export default function DeleteRoom({ children }) { return ( <AlertDialog> <AlertDialogTrigger>{children}</AlertDialogTrigger> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle style={{ color: "#2AC3DE" }}>Are you sure?</AlertDialogTitle> <AlertDialogDescription> This will delete the room and kick everyone from it. </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> <AlertDialogCancel>Cancel</AlertDialogCancel> <AlertDialogAction>Continue</AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> </AlertDialog> ) }
This for example couple be a reusable component
Not really, The Alert Dialog components are composable UI components from Shadcn UI that's used to build the components specific to our domain. DeleteRoom is reusable which is what we need. Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
Now of course, the hard coded texts can be replaced dynamically by props but as of now, doing that is not needed yet.
This is what I'm talking about. The code between deleteProject.tsx and deleteRoom.tsxI don't think we have to do that. DRY is a a best practice but should be used wisely. if we have a dynamic component for example
DestructiveAction
and then give it props for the components of the dialog, sure that'd be reusable but that might be premature abstraction because we may not have a lot of destructive actions. Readability may also be affected as the Dev would have to figure out what type of action the component is for versus if the component name is exactly what it doesDeleteRoom
. I'd say this is not a big deal and personally think the code's fine as it is right now.Sort of, I would at least rip out the layout into a base Alert component that accepts some props. Then you could still have the deleteRoom.tsx components etc if you needed reuse. No need to make it too complicated with logic. The main thing is keeping it DRY.
import BaseAlert from './BaseAlert'; type DeleteRoomProps = { children: React.ReactNode; }; const DeleteRoom = ( { children }: DeleteRoomProps ) => ( <BaseAlert title='Are you sure?' description='This will delete the room and kick everyone from it.' cancelButtonText='Cancel' actionButtonText='Continue' > {children} </BaseAlert> ); export default DeleteRoom;
Something like that
what's the point of children? Where would children go into in
BaseAlert
?Check out the code in the PR.
children
is being used for a lot. I'm not questioning it though :) It's also referenced in this quote chain at the top
yeah my bad, missed that
It's a good idea to remove your old code instead of commenting it out, its easy to go back through git as its used to version.
It's a good idea to remove your old code instead of commenting it out, its easy to go back through git as its used to version.
Will do
I think I've fixed all of the merge conflicts.
Modified some things including the components.json file so that when you install ShadCN UI components via command line it installs in the right place. Also made a cookie manager than can get, set and remove cookies.
Added some parts in the App.tsx file for testing these dialogs. It stays nice and consistent using the colours in the Figma file throughout. Also changed the title in the editor to say "CO-DE - Editor" because it looked nicer than the default "Vite + React + TS" Added dialogs and alert dialogs (Some of them did not follow the designs, some of them don't even exist in the Figma file):