Allow adding multiple URLs at once #158 #167
Conversation
|
Someone is attempting to deploy a commit to the Mohamed Bassem's projects Team on Vercel. A member of the Team first needs to authorize it. |
| const [open, setOpen] = React.useState(false); | ||
| const [importData, setImportData] = React.useState(initialImportData); |
There was a problem hiding this comment.
There's some room for simplification here:
- Have only one state which is
multiURLImportState. This state can be either set when we detect a multi-url input, or null if we didn't (with null being the default)`. - Remove the
opencontrol state for the dialog and instead only render the dialog ifmultiURLImportStateis not null. In react, you typically do that with(multiURLImportState && <MultiInputDialog> ...). The dialog will need to setdefaultOpen=trueso that it's open when it's rendered. - When the user takes an action or
onOpenChange, just setmultiURLImportStateto null which will remove the dialog.
What do you think?
| children?: React.ReactNode; | ||
| }) { | ||
| return ( | ||
| <Dialog defaultOpen={true}> |
There was a problem hiding this comment.
Given that this is a generic dialog (in the ui dir), you'll want to make this as a prop.
There was a problem hiding this comment.
ok now I am confused. If the defaultOpen will no longer be true, then I will have to bring back the open function, otherwise this control will never show?
I was under the impression the goal was to make this always show, otherwise don't you have 2 ways of opening and closing it? (one with calling the open function and 1 by configuring it with defaultOpen=true)
There was a problem hiding this comment.
@kamtschatka Your new component is a generic dialog. So you'll need to pass params similar to that of the dialog to allow the different usecases to customize it. In that case, you'll want to expose open?, onOpenChange? and defaultOpen? (and forward them to Dialog). Then our usecase (which is the confirmation of multi URL), is the one that won't pass open, but will set defaultOpen to true, does that make sense?
There was a problem hiding this comment.
sure, i was just confused that above you said to remove the open control state and below you said open is no longer used.
So basically it was good as it was before, the idea was more to add an additional way to allow showing the dialog without calling setOpen.
There was a problem hiding this comment.
open should continue being there, but you'll need to make it optional and pass it to the dialog. Currently, it's there, but not being used.
The whole suggestion for simplification was yes to not have a controlled open property for the dialog. We just render it when needed it, and un-render it when it's no longer needed. All of that while keeping the option to use MultiInputDialog in a controlled fashion in a different place if we want to (aka keeping it generic)
There was a problem hiding this comment.
ok, but then I have to get rid of the defaultOpen again, because then I would have the open parameter and the defaultOpen parameter, where 1 could be true and 1 could be false, which would be weird.
I'll just pass true to open, without having a state to keep it, because that is implied by the MultiUrlImportState.
There was a problem hiding this comment.
@kamtschatka you need both. For example this the API of the dialog component (https://www.radix-ui.com/primitives/docs/components/dialog#root) we're using. As you can see, it has both.
open is a controlled param. If you pass open to true (instead of defaultOpen) people won't be able to close the dialog at all. defaultOpen just initializes the initial state of the dialog.
So in our case, we need defaultOpen=true but we need to tell the dialog component that it's not a controlled dialog by setting open=undefined in other words, not passing open.
There was a problem hiding this comment.
but we are controlling the rendering state through {multiUrlImportState && (...., so if we don't want the dialog to show, we are not even rendering it.
And as for reusability: if you want to control the open state, then you pass in a proper state?
I will push again, for you to review, I am very convinced that the defaultOpen AND open state are not required at the same time.
There was a problem hiding this comment.
I think functionality wise, this looks good. But with the current MultiInputDialog implementation, users won't be able to do an uncontrolled dialog with a trigger. I'll followup with a small PR to show you what I had in mind.
Added a reusable dialog opening a dialog that allows you to decide if you want to import multiple URLs at once if you provide only that
Added a reusable dialog
opening a dialog that allows you to decide if you want to import multiple URLs at once if you provide only that