Skip to content

Conversation

@SebayK
Copy link
Contributor

@SebayK SebayK commented Dec 9, 2025

fix(zIndex-execution-during-local-init): update zIndex handling during initialization and extend tests

What I did

  1. update zIndex handling during initialization
  2. extend zIndex tests in OverlayController.test.js

Copilot AI review requested due to automatic review settings December 9, 2025 04:50
@changeset-bot
Copy link

changeset-bot bot commented Dec 9, 2025

⚠️ No Changeset found

Latest commit: dfe1065

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@SebayK
Copy link
Contributor Author

SebayK commented Dec 9, 2025

Resolve #2608

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Member

@tlouisse tlouisse left a comment

Choose a reason for hiding this comment

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

Thanks! 🙏

Can you maybe remove the checks with expect(ctrl.manager.shownList).to.have.lengthOf(1); ?
Or are thet relevant within these z-index tests?

await ctrl.show();
// @ts-expect-error find out why config would/could be undfined
expect(ctrl.content.style.zIndex).to.equal(`${ctrl.config.zIndex + 1}`);
expect(ctrl.manager.shownList).to.have.lengthOf(1);
Copy link
Member

Choose a reason for hiding this comment

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

PR looks good to me.
Any reason we should check for the shownList length here in these tests, instead of in the manager? https://github.com/ing-bank/lion/blob/master/packages/ui/components/overlays/test/OverlaysManager.test.js#L83

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f I understand correctly, if there is more siblings, it always should have higher zIndex than the previous sibling by one. It's probably a little more than that. Let me know

@tlouisse
Copy link
Member

@SebayK we discussed this in person, but I leave a comment here for transparency...

This one needs a bit more investigation. I see we applied zIndex on 3 nodes (should have been one I think).

Also: now that we moved to dialog, (n case it's configured as modal) it paints to the top layer. It will win from every other layer regardless of zIndex (that's what I expect). I think this should somehow be reflected in the tests. (this will play an even bigger role once we move to popovers for (local) overlays)

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.

2 participants