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

Is 150 fix feature tour zindex #1252

Merged
merged 2 commits into from
May 2, 2023
Merged

Conversation

harishv7
Copy link
Contributor

@harishv7 harishv7 commented Apr 28, 2023

Problem

This fix replaces the temporary hotfix for the feature tour conflict with the CMS FE modals (see #1250)

Closes IS-150

Solution

Each component has a different zIndex. The feature tour elements should have a zIndex between that of the elements on CMS dashboard and workspace and the modals. This will prevent users from interacting with the feature tour components when modals are open

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible

Testing

Run this locally and ensure you are able to use both the feature tour and modals without conflicts

@harishv7 harishv7 temporarily deployed to staging April 28, 2023 07:20 — with GitHub Actions Inactive
@harishv7 harishv7 temporarily deployed to staging April 28, 2023 07:31 — with GitHub Actions Inactive
@harishv7 harishv7 temporarily deployed to staging April 28, 2023 07:34 — with GitHub Actions Inactive
zIndex: 2000, // need this for it to be on top of the navbar
zIndex: 1100, // need this for it to be on top of the navbar
Copy link
Contributor

Choose a reason for hiding this comment

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

@harishv7 in the hotfix PR, there is no change to the zIndex value. In both master and develop branches, the value is 2000 now, why are we changing it to 1100?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QiluXie The hotfix PR tried to monitor the state as to if a modal is open or not to determine whether we need to show the feature tour. Bu that is not required.

The way we should handle this is by ensuring modals have a higher zIndex (which means they overlay the feature tour).

At the same time, we need to ensure feature tour has a zIndex which is more than the nav bar and the dashboard/workspace components.

1100 is a sweet spot between the 2 values which will allow feature tour to show up on dashboard/workspace "above" the components and will hide feature tour when a modal is opened (since modals will have a higher zIndex)

Copy link
Contributor Author

@harishv7 harishv7 Apr 28, 2023

Choose a reason for hiding this comment

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

Screenshot 2023-04-28 at 6 39 22 PM

Screenshot 2023-04-28 at 6 40 29 PM

Review Request modal has a zIndex of 1400

Copy link
Contributor Author

@harishv7 harishv7 Apr 28, 2023

Choose a reason for hiding this comment

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

Screenshot 2023-04-28 at 6 42 35 PM

Create new folder modal has 1300

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 tried using 1200, but it didn't work as there are other components with 1200. Hence 1100 was the option I went with

Copy link
Contributor

@kishore03109 kishore03109 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 this better solution!

@harishv7 harishv7 merged commit bc2ceaa into develop May 2, 2023
@harishv7 harishv7 deleted the is-150-fix-feature-tour-zindex branch May 2, 2023 05:08
@mergify mergify bot requested a review from a team June 1, 2023 05:09
@mergify
Copy link

mergify bot commented Jun 1, 2023

This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers

1 similar comment
@mergify
Copy link

mergify bot commented Jul 1, 2023

This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers

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.

3 participants