[core] fix(Dialog): set dialog header z-index to 0

[evansjohnson]

Changes proposed in this pull request:

remove z-index from dialog-header class

Reviewers should focus on:

any reasons this high z-index may be needed? without more context it seems unlikely the highest constant z-index should be for the dialog header but I could be missing something.

Screenshot

gif showing:

• header overlaps select if usePortal is false in popoverProps
• z-index: 0 is needed for MultistepDialog - the PR that introduced this component added this z-index (see here) and I think it was just set higher than it needed to be
• https://lvepb.csb.app/ should be a working demo
• https://codesandbox.io/s/dialog-header-overlap-reproducer-lvepb?file=/src/index.tsx should let you play around/examine sandbox code

[evansjohnson]

realized the scss variable probably can't be removed until v4 in case a consumer is importing it - curious if that's the right approach here

[adidahiya]

Why do you need usePortal={true} for your select?

[evansjohnson]

it's hard to tell working with legacy code, but it seemed there was a clear path forward to fix the issue here from playing around in the console

regardless of this specific issue the header having z-index: 30 could cause other issues and doesn't seem warranted

[adidahiya]

Well the intention of the header z-index was to have it appear above scrolling dialog body content. It was not intended to interact with portal'd / popover elements, which are supposed to break out of the current stacking context completely.

[evansjohnson]

the header has display: flex unless it was being absolutely positioned I'm not sure why it would need z-index, the scrolling for body content happens in an element positioned below the header as far as I can tell - perhaps you have something else in mind for the case where the z-index is used to stay over body content?

looking at https://blueprintjs.com/docs/#core/components/dialog and making my window short it seems like dialog body doesn't add scrolling and instead adds a scrollbar at the window level when the dialog overflows

I'm not sure if the usage of usePortal is correct but it is an option provided by the library so I'd expect it to play nicely with other components. Considering the case of a custom absolutely positioned element, I don't see why I should have to realize this header has a z-index and to make sure my component has a higher z-index

[evansjohnson]

hey @adidahiya - are there any concerns with this PR? sorry if the repro makes this look overwhelming, I was trying to thoroughly demonstrate the issue and that this 1 line change fixes it (toggling the 1 line change with the "style override" checkbox and manually overriding the css in the repro when checked)

this change keeps users from needing to fight against blueprint setting an unnecessarily high z-index for the dialog header - the second part of the repro shows the z-index was added for the border in the multistep dialog and was not needed before that, and that "0" also works there

[evansjohnson]

could leave a comment above $pt-z-index-dialog-header now that blueprint no longer uses it to remove in a future breaking version

[adidahiya]

On second thought... this PR looks good. I missed the fact that this z-index was only added in the MultiStepDialog PR, and you're right that it doesn't need to be set to such a high value.