Skip to content

Comments

Refactor Dialog and Popup to remove jquery#945

Open
jernestmyers wants to merge 3 commits intomainfrom
debt/jquery-popup-instance
Open

Refactor Dialog and Popup to remove jquery#945
jernestmyers wants to merge 3 commits intomainfrom
debt/jquery-popup-instance

Conversation

@jernestmyers
Copy link
Contributor

@jernestmyers jernestmyers commented Mar 18, 2024

Works to resolve the second wdk-client task from #899: Overlays/Popup: used only in Dialog, but Dialog is used widely

It's worth noting that the draggable prop passed to Dialog was being ignored. Whether or not draggable was passed into Dialog and thus down to Popup, it was always rendered as draggable. With these changes, I'm listening to the draggable prop and only rendering a draggable Dialog when it's explicitly passed in.

@jernestmyers jernestmyers marked this pull request as ready for review August 26, 2024 14:54
@jernestmyers jernestmyers requested a review from dmfalke August 26, 2024 14:54
Copy link
Contributor

@dmfalke dmfalke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great. However, I think we need to make the component draggable by default. There are some places where we definitely want the dialog to be draggable, even though we don't specify it. For example, the step details dialog in the strategy panel.

If you have the time and energy, you could find all of the places where we use the Dialog component and we can determine if each one should be draggable. Otherwise, let's just make it the default behavior.

@jernestmyers
Copy link
Contributor Author

I think this looks great. However, I think we need to make the component draggable by default. There are some places where we definitely want the dialog to be draggable, even though we don't specify it. For example, the step details dialog in the strategy panel.

If you have the time and energy, you could find all of the places where we use the Dialog component and we can determine if each one should be draggable. Otherwise, let's just make it the default behavior.

I opted for the simpler solution of defaulting to true. That's essentially how it was working before, though now I suppose we'll needlessly be passing draggable=true to some components.

@jernestmyers jernestmyers requested a review from dmfalke August 27, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants