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

Anchored region #530

Merged
merged 15 commits into from
May 4, 2022
Merged

Anchored region #530

merged 15 commits into from
May 4, 2022

Conversation

mollykreis
Copy link
Contributor

Pull Request

🤨 Rationale

Create nimble-anchored-region component.

👩‍💻 Implementation

Created a light-weight wrapper around the FAST anchored-region.

🧪 Testing

  • Added unit tests for the nimble-anchored-region being returned from tagFor(FoundationAnchoredRegion) and the element being able to be constructed
  • Added matrix tests for the component
    • Note: for technical reasons and because nothing in the anchored region is theme aware, I did not create matrix stories for each theme
  • Added storybook story for the component

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

:host {
contain: layout;
display: block;
z-index: 1;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the select and the drawer are also configuring z-index. Do we know how these are impacting eachother? https://cs.github.com/ni/nimble?q=z-index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set this to 1 to match the select, and I tested that it worked correctly when put inside of a drawer. However, I just tested the menu button within a grid, and I realized that I need to increase the z-index because the header row in the grid has a z-index of 200.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm changing this to 1000. I tested that the menu button looked correct in SystemLink when it was in a grid's toolbar, in a grid's context menu, in the header, in a drawer, and on a generic page.

Copy link
Member

@rajsite rajsite May 4, 2022

Choose a reason for hiding this comment

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

Where is 1000 coming from? Is that because it's one more than the drawer? Or it's arbitrarily higher than the 200 z-index in the grid?

Unfortunately FAST doesn't have a system for managing z-index values we can copy.

Could we either in this PR or a follow-on (if a follow-on we should make an issue to track it) make a global enum of the different z-index states we use in our styles (like low, medium, high or something) and can include some docs on how the value is derived (ie we chose 1000 because it's larger than the external grid we have compatibility with)?

Maybe we can eventually have some tokens exported that external consumers can use to align z-indexes with nimble. (or even better longer term we can use the dialog element, etc to do the layering for us without the need for z-index)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I picked something that was arbitrarily higher than the 200 z-index in the grid. We don't need it to be higher than the 999 set on the drawer.

I'll create an issue for making a global enum of z-indexes and address it in separate PR. I'm not sure I see exactly what your vision is, so I'd rather have that discussion on a different PR and not delay this one being merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created issue: #539

@mollykreis mollykreis requested a review from rajsite May 4, 2022 15:49
@mollykreis mollykreis merged commit 653341a into main May 4, 2022
@mollykreis mollykreis deleted the anchored-region branch May 4, 2022 21:17
@jattasNI jattasNI mentioned this pull request Jan 3, 2024
1 task
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.

3 participants