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

fix: Export popup has small z-index [INS-3640] #7187

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

CurryYangxx
Copy link
Member

@CurryYangxx CurryYangxx commented Mar 18, 2024

Close #7176

Changes

  • remove the global variable(globalZIndex) in base modal component
  • add a new hook to admin every modal z-index(Even without using the base modal component)
  • change the export request modal z-index, use globalModalZIndex instead

How to test

  • Click Preferences → Data -> Export the collection, and you can see the export modal

@CurryYangxx CurryYangxx requested review from ihexxa and a team March 18, 2024 07:22
ihexxa
ihexxa previously approved these changes Mar 18, 2024
Copy link
Contributor

@ihexxa ihexxa left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix, good job👍.

@CurryYangxx CurryYangxx requested a review from a team March 18, 2024 08:43
@jackkav jackkav requested a review from gatzjames March 18, 2024 09:28
Copy link
Contributor

@jackkav jackkav left a comment

Choose a reason for hiding this comment

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

Rather than syncing the zlndex's with a hook we could try to remove the zindex settings? @gatzjames ?

Copy link
Contributor

@gatzjames gatzjames left a comment

Choose a reason for hiding this comment

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

We are moving away from the base/modal and using react-aria-components modals instead.
I'd suggest changing the settings-modal.tsx to have the same z-index as other modals instead.

In settings-modal.tsx line 44

<Modal className='!z-10' ref={modalRef} tall {...props}>
   ...
</Modal>

That way the last modal to open will always be on top and there's no need to keep calculations in state.

@CurryYangxx
Copy link
Member Author

We are moving away from the base/modal and using react-aria-components modals instead. I'd suggest changing the settings-modal.tsx to have the same z-index as other modals instead.

In settings-modal.tsx line 44

<Modal className='!z-10' ref={modalRef} tall {...props}>
   ...
</Modal>

That way the last modal to open will always be on top and there's no need to keep calculations in state.

This way works. And do we have other similar scenes that import the base/modal? I'm worried that if there are similar scenes that are not covered by the same z-index, there may be the same issue. @gatzjames

@gatzjames
Copy link
Contributor

This way works. And do we have other similar scenes that import the base/modal? I'm worried that if there are similar scenes that are not covered by the same z-index, there may be the same issue. @gatzjames

There are definitely more modals that use the previous approach.
I don't think there are other cases where we have this issue especially since opening a modal on top of another modal is offering a bad UX. So our goal is to eliminate these issues by migrating these modals instead of investing more on the previous approach

@CurryYangxx CurryYangxx merged commit a5f859f into develop Mar 19, 2024
7 checks passed
@CurryYangxx CurryYangxx deleted the fix/export-modal-issue branch March 19, 2024 10:08
@CurryYangxx CurryYangxx changed the title fix: Export popup has small z-index fix: Export popup has small z-index [INS-3640] Mar 25, 2024
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.

Export popup has small z-index
4 participants