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

#332 z-index for Popover component #398

Merged
merged 4 commits into from
Sep 9, 2022
Merged

Conversation

MichalPaszowski
Copy link
Contributor

@MichalPaszowski MichalPaszowski commented Aug 26, 2022

Resolves: #332

Description

It seems to be enough to fix stuff like:
https://monosnap.com/file/agb5EyL9CrbJqO8E0vNQhtUAo6BYOo

Storybook

https://613a8e945a5665003a05113b-iaeypnmfan.chromatic.com/

Checklist

Obligatory:

  • Self-review
  • Unit & integration tests
  • Storybook cases
  • Design review
  • Functional (QA) review

Optional:

  • Accessibility cases (keyboard control, correct HTML markup, etc.)

@MichalPaszowski MichalPaszowski changed the title set zindex on popover #332 z-index for Popover component Aug 26, 2022
@MichalPaszowski MichalPaszowski added this to the v1.0.0 milestone Aug 26, 2022
@MichalPaszowski MichalPaszowski self-assigned this Aug 26, 2022
@@ -5,6 +5,7 @@
display: none;
max-width: 336px;
min-width: 168px;
z-index: 1000;

Choose a reason for hiding this comment

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

Could we use a dedicated variable from
https://github.com/livechat/design-system/blob/39fcca44937242ba3bd3bc7c8b855269d335b2f1/packages/react-components/src/utils/StackingContextLevel.scss

I believe we would need to introduce a new level:

W could follow this concept from LiveChat's Agent App:

export enum StackingContextLevel {
  Tooltip = 1000,
  Dropdown = 2000,
  ...
}

@@ -5,6 +5,7 @@
display: none;
max-width: 336px;
min-width: 168px;
z-index: $stacking-context-level-popover;
Copy link

Choose a reason for hiding this comment

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

You need to import it before usage @import '../../utils/StackingContextLevel';

Copy link

@sgraczyk sgraczyk left a comment

Choose a reason for hiding this comment

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

Build fails, the variable is not imported

@MichalPaszowski MichalPaszowski merged commit 545948b into v1 Sep 9, 2022
@MichalPaszowski MichalPaszowski deleted the popover/zindex-fix branch September 9, 2022 12:24
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.

[Popover] should have z-index set to prevent content overlapping
3 participants