Skip to content

fix: added z-index to mode help tooltip and z-index system (issue #298) #544

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

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

Conversation

dpolevodin
Copy link

@dpolevodin dpolevodin commented Dec 27, 2024

fixes #298
z-index for the sticky-toolbar has been lowered from 2000 to 990, since the HelpMark tooltip index is set to 1000 in uikit
Added z-index system and updated readme.
Changed current z-index values in project by z-index mixin

image

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@dpolevodin dpolevodin changed the title fix: added z-index to mode help tooltip and z-index system #298 fix: added z-index to mode help tooltip and z-index system (issue #298) Jan 13, 2025

.tooltip {
@include mixins.z-index('forefront');
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

add to docs plz too

'img-settings-button': 2,
'table-view-button': 100,
'table-cell-button': 110,
'sticky-toolbar': 990,
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. header sticky toolbar+
  2. header sticky toolbar
  3. context toolbar+ (popups, hints)
  4. context toolbar
  5. suggest popup+ (for hints), (... buttons menu)
  6. suggest popup
  7. block panel popup

context is popup from text selection
suggest is /, :

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you need to change z-index in uikit component use style https://github.com/gravity-ui/uikit/blob/main/src/components/types.ts#L4C5-L4C27

like this:

style={{
   // ...otherStyles,
   zIndex: var(--md-editor-z-index-level-suggest);
}}

@makhnatkin
Copy link
Collaborator

@dpolevodin could you please outline the steps you believe we should take to solve the task at this stage?

@dpolevodin
Copy link
Author

dpolevodin commented Apr 17, 2025

@dpolevodin could you please outline the steps you believe we should take to solve the task at this stage?
@makhnatkin Hi! I would split the task into two parts:

  1. Solving the bug when a popup with hints from HelpMark does not overlap the toolbar in the sticky state.
    I see the solution to this problem in adding a zIndex prop to the Popover component, since HelpMark uses popoverProps. But for some reason, popoverProps did not include the zIndex property when inheriting properties from PopupProps. I updated the PR using the required zIndex variable for HelpMark inside popoverProps. In fact, this solution works and the required zIndex is applied to the popover from HelpMark, but it causes a TypeScript error: TS2353: Object literal may only specify known properties, and zIndex does not exist in type Omit<PopoverProps, "children"> precisely because the properties were not added to the ui-kit Popover component. I am attaching a screenshot
    image

Now, it works correctly on UI:
image

@dpolevodin
Copy link
Author

@dpolevodin could you please outline the steps you believe we should take to solve the task at this stage?
@makhnatkin Hi! I would split the task into two parts:

  1. Solving the bug when a popup with hints from HelpMark does not overlap the toolbar in the sticky state.
    I see the solution to this problem in adding a zIndex prop to the Popover component, since HelpMark uses popoverProps. But for some reason, popoverProps did not include the zIndex property when inheriting properties from PopupProps. I updated the PR using the required zIndex variable for HelpMark inside popoverProps. In fact, this solution works and the required zIndex is applied to the popover from HelpMark, but it causes a TypeScript error: TS2353: Object literal may only specify known properties, and zIndex does not exist in type Omit<PopoverProps, "children"> precisely because the properties were not added to the ui-kit Popover component. I am attaching a screenshot
    image

Now, it works correctly on UI: image

@dpolevodin could you please outline the steps you believe we should take to solve the task at this stage?

The second part. It is necessary to work out the solution for layers. In general, I see two options:

  1. Leave it approximately as it is now in the PR, perhaps think about more generalized names of variables, but in essence, it will be in the form of storing information about the zIndex value for each layer in one file and further adding the necessary index in the component styles using a mixin, as indicated in the PR in Readme, example:
.tooltip { 
	@include mixins.z-index('forefront'); 
}
  1. Another option, which is more complex and "heavy", in my opinion in terms of performance, is to create a context and a hook for managing zIndex inside a react component. It might look something like this
import React, { createContext, useContext, useState } from 'react';

type ZIndexContextType = {
  getZIndex: (layer: string) => number;
  bringToFront: (layer: string) => void;
};

const ZIndexContext = createContext<ZIndexContextType | undefined>(undefined);

export const ZIndexProvider: React.FC<{children: React.ReactNode}> = ({ children }) => {
  const [layers, setLayers] = useState<Record<string, number>>({
    deep: -1,
    default: 1,
    dropdown: 100,
    modal: 500,
    notification: 700,
    tooltip: 800,
    max: 9999
  });

  const bringToFront = (layer: string) => {
    setLayers(prev => {
      const currentMax = Math.max(...Object.values(prev));
      return {
        ...prev,
        [layer]: currentMax + 1
      };
    });
  };

  const getZIndex = (layer: string) => layers[layer] || 1;

  return (
    <ZIndexContext.Provider value={{ getZIndex, bringToFront }}>
      {children}
    </ZIndexContext.Provider>
  );
};

export const useZIndex = () => {
  const context = useContext(ZIndexContext);
  if (!context) {
    throw new Error('useZIndex must be used within a ZIndexProvider');
  }
  return context;
};
```
And the use in the component will probably look like this, using the example of a modal window:

// Modal.tsx
import React, { useEffect } from 'react';
import { useZIndex } from './ZIndexContext';
import './Modal.scss';

export const Modal: React.FC = ({ children }) => {
const { getZIndex, bringToFront } = useZIndex();
const zIndex = getZIndex('modal');

useEffect(() => {
bringToFront('modal');
}, []);

return (
<div className="modal" style={{ zIndex }}>
{children}

);
};

And you will also need to wrap the application in ZIndexProvider like this:
```
export const App = () => {
  return (
    <ZIndexProvider>
      {/* app */}
      <Modal>Modal example</Modal>
    </ZIndexProvider>
  );
};

In my opinion, the second option has a complicated logic, in itself can affect the performance due to calling additional hooks on the UI and setting zIndex via style and, possibly, cause additional problems. Although it may look like a more dynamic and flexible solution.

Perhaps you will have even better suggestions :)

@dpolevodin
Copy link
Author

dpolevodin commented Apr 23, 2025

Steps to solve the issue:

  1. Add zIndex for PopupProps to ui-kit [ x ]
  2. Update the ui-kit library to the new version with the necessary changes after release [ ]
  3. Collect test cases with screenshots of many open popups/tooltips to determine zIndex values ​​and identify possible problems with incorrect layer order [ ]
  4. Based on the received cases, select the implementation option for the zIndex system:
  • storing information about the zIndex value for each layer in one file and further adding the necessary index in the
    component styles using a mixin
    or
  • create a context and a hook for managing zIndex inside a react component [ ]
  1. Update PR with the selected solution [ ]

@makhnatkin
Copy link
Collaborator

@dpolevodin lets collect test cases with screenshots of many open popups/tooltips to determine zIndex values ​​and identify possible problems with incorrect layer order

@gravity-ui-bot
Copy link
Contributor

Visual Tests Report is ready.

@dpolevodin
Copy link
Author

@dpolevodin lets collect test cases with screenshots of many open popups/tooltips to determine zIndex values ​​and identify possible problems with incorrect layer order

@makhnatkin Hi! yes, this is the next step. I will collect it soon

Полеводин Дмитрий Игоревич (4094029) added 2 commits May 14, 2025 18:42
@dpolevodin
Copy link
Author

dpolevodin commented May 14, 2025

@makhnatkin I merged main branch to the working branch, upped @gravity-ui/uikit version to latest and added zIndex prop to PopoverProps in HelpMark to fix the error with sticky toolbar overlay over the popover
image

  • The next step I plan to take is to collect cases of working out layers when opening the toolbar tools when it is in the normal position and sticky position

@dpolevodin
Copy link
Author

dpolevodin commented May 27, 2025

3. Collect test cases with screenshots of many open popups/tooltips to determine zIndex values ​​and identify possible problems with incorrect layer order:

  1. header sticky toolbar+/toolbar
  • HelpMark (zIndex: 1000) with text variants (under sticky toolbar)
    image
  • text color popup (zIndex: 1000) (under sticky toolbar)
    image
    -Heading popup (zIndex: 100) (under sticky toolbar)
    image
  • List popup (zIndex: 1000) (under sticky toolbar)
    image
  • tools tooltip (zIndex: 10_000) (before sticky toolbar)
    image
  • file upload form (popup) (zIndex: 1000) (under sticky toolbar)
    image
  • link popup (zIndex: 1000) (under sticky toolbar)
    image
  • code popup (zIndex: 1000) (under sticky toolbar)
    image
  • image upload popup (zIndex: 1000) (under sticky toolbar)
    image
  • more action popup (zIndex: 1000) (under sticky toolbar)
    image
  • popup from Heading tool items
image

@dpolevodin dpolevodin closed this Jun 2, 2025
@dpolevodin dpolevodin force-pushed the fix/tooltip-hidden-under-toolbar branch from a08425b to 0e1415c Compare June 2, 2025 09:16
@dpolevodin dpolevodin reopened this Jun 2, 2025
@dpolevodin
Copy link
Author

dpolevodin commented Jun 2, 2025

3. Collect test cases with screenshots of many open popups/tooltips to determine zIndex values ​​and identify possible problems with incorrect layer order

context toolbar+ (popups, hints):

  • Text color context popup before toolbar
image - Text color context popup behind sticky-toolbar image - Text context popup before toolbar image - Text context popup behind sticky-toolbar image - Text context popup (item tooltip) before sticky-toolbar image - Context items hint before sticky toolbar image - Context link form popup behind sticky toolbar image - Context link form popup before toolbar image - Context collapsing section flag tooltip before sticky toolbar/toolbar image

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.

Help tooltip is hidden under the toolbar on small screens
3 participants