Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Dialog and Popup to remove jquery #945

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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
Copy link
Member

@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.

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