Skip to content
This repository has been archived by the owner on Jul 24, 2021. It is now read-only.

Communities hotfix #395

Merged
merged 3 commits into from
Mar 14, 2019
Merged

Communities hotfix #395

merged 3 commits into from
Mar 14, 2019

Conversation

ash3rz
Copy link
Contributor

@ash3rz ash3rz commented Mar 13, 2019

This will:

  • In the Apps window, deselect communities after the user has submitted a search
  • In the Communities window, fix issues regarding searching for apps to add to a community

ash3rz and others added 3 commits March 13, 2019 10:17
Opening a GWT dialog from a React modal dialog causes z-index issues and focus issues, leading the GWT dialog to miss click events
Copy link
Member

@psarando psarando left a comment

Choose a reason for hiding this comment

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

LGTM, though I had a suggestion for hiding the React dialog while the GWT dialog is open.

@@ -76,6 +84,7 @@ class EditCommunityDialog extends Component {
fullWidth={true}
maxWidth='lg'
onClose={onClose}
classes={{root: isSelectAppsDlgOpen ? classes.hidden : null}}
Copy link
Member

@psarando psarando Mar 13, 2019

Choose a reason for hiding this comment

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

In my metadata dialog PR (#394), I set the dialog's open prop to false to hide it temporarily, and when it needs to come back, resetting its open prop to true shows it again, but it retains its state while it's hidden: https://github.com/cyverse-de/ui/pull/394/files#diff-80f7a61599153f473b4b30c76a63f831R74

This also has the advantage that it will also use its slide up/down animation when hiding and re-displaying. I'm not sure if a transition animation is used when setting this CSS visibility attribute.

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem to be working for me, probably because I have state inside EditCommunity and that seems to automatically get cleared when EditCommunityDialog isn't open. The constructor gets called again when the dialog reopens. I could take this opportunity to refactor the state up the hierarchy if you guys don't mind waiting for the hotfix.

Copy link
Member

Choose a reason for hiding this comment

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

That's unfortunate 😩 I noticed the EditCommunityDialog uses the EditCommunity component as its dialog contents, which is actually recommended for performance:
https://material-ui.com/demos/dialogs/#performance

I was also planning on refactoring the metadata dialogs this way to see if that will help the form's performance, but if hiding the dialog also removes its children (resetting their state), then that could make the refactoring a trickier task.

Pulling state up to the dialog does seem like it might help, but I'm not sure if that makes sense (e.g. maybe EditCommunity should be able to display in something besides a dialog). It also might be too much work for this hotfix, so I'm fine if we :shipit: as is 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know 😩 There is a "fix" (mui/material-ui#10572), and I tried that, but then it keeps those children in the DOM forever. So in my case, if I opened the dialog, closed it, closed the Communities window too, and then reopened everything, I could add an infinite number of dialogs to the DOM that just stay there forever (even though I don't see them) and create duplicate static IDs.

@ash3rz
Copy link
Contributor Author

ash3rz commented Mar 14, 2019

Thanks for the reviews!

@ash3rz ash3rz merged commit 9490e89 into cyverse-archive:master Mar 14, 2019
@ash3rz ash3rz deleted the hotfix branch March 14, 2019 16:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants