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.

Sign up for GitHub

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

Jump to bottom

[Popover] Add zIndexOverride prop #3937

Merged
merged 3 commits into from
Mar 10, 2021
Merged

[Popover] Add zIndexOverride prop #3937

merged 3 commits into from
Mar 10, 2021

Conversation

kyledurand
Copy link
Contributor

WHY are these changes introduced?

Fixes https://github.com/Shopify/polaris-ux/issues/602

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx: 
import React from 'react';

import {Page, Popover} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <Popover
        active
        activator={<div>asdf</div>}
        onClose={() => {}}
        zIndexOverride={402}
      >
        <ul>
          <li>one</li>
          <li>two</li>
        </ul>
      </Popover>
      <div style={{background: 'red', zIndex: 401, position: 'relative'}}>
        some content
      </div>
    </Page>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@kyledurand kyledurand force-pushed the popover-custom-zIndex-override branch 3 times, most recently from c8558fb to 313d033 Compare January 28, 2021 21:20
@github-actions
Copy link
Contributor

github-actions bot commented Jan 28, 2021 •
edited
Loading

🟡 This pull request modifies 7 files and might impact 62 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:
All files potentially affected (total: 62)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Popover/Popover.tsx (total: 58)

Files potentially affected (total: 58)

🧩 src/components/Popover/components/PopoverOverlay/PopoverOverlay.tsx (total: 59)

Files potentially affected (total: 59)

🧩 src/components/Popover/components/PopoverOverlay/tests/PopoverOverlay.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Popover/tests/Popover.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/PositionedOverlay/PositionedOverlay.tsx (total: 62)

Files potentially affected (total: 62)

🧩 src/components/PositionedOverlay/tests/PositionedOverlay.test.tsx (total: 0)

Files potentially affected (total: 0)

@kyledurand kyledurand force-pushed the popover-custom-zIndex-override branch from 313d033 to 8d7750c Compare January 28, 2021 21:22
@Bananajunk
Copy link
Contributor

Need to do some testing when I get a chance, but at a glance does seem like this would solve our underlying issue

Bananajunk
Bananajunk reviewed Feb 2, 2021
View reviewed changes
src/components/PositionedOverlay/PositionedOverlay.tsx
} = this.props;

const style = {
top: top == null || isNaN(top) ? undefined : top,
left: left == null || isNaN(left) ? undefined : left,
right: right == null || isNaN(right) ? undefined : right,
width: width == null || isNaN(width) ? undefined : width,
zIndex: zIndex == null || isNaN(zIndex) ? undefined : zIndex,
zIndex: zIndexOverride || zIndex || undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of zIndex typing (number | null) we shouldn't need the isNaN check? So this simplifies that condition statement I believe.

@Bananajunk
Copy link
Contributor

Brought these changes over locally into the Flow-App repo (where the request initiated from) and can confirm this fixes our issue and allows us to set a custom z-index value for our DatePicker Popover. 🎩

@kyledurand kyledurand marked this pull request as ready for review February 8, 2021 16:02
@Bananajunk Bananajunk force-pushed the popover-custom-zIndex-override branch from 8e0002d to 7e447f5 Compare March 3, 2021 15:25
@Bananajunk Bananajunk force-pushed the popover-custom-zIndex-override branch from 7e447f5 to 6046a5e Compare March 9, 2021 19:14
@kyledurand kyledurand merged commit 5eaf263 into main Mar 10, 2021
@kyledurand kyledurand deleted the popover-custom-zIndex-override branch March 10, 2021 14:27
This was referenced Mar 18, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bananajunk Bananajunk Bananajunk approved these changes

@alex-page alex-page alex-page approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kyledurand @Bananajunk @alex-page