Skip to content

Conversation

@BumbleB2na
Copy link
Collaborator

@BumbleB2na BumbleB2na commented Jan 20, 2025

Also see related ui-components-docs PR: GovAlta/ui-components-docs#305

Before (the change)

Popover had prop: "relative" that when enabled would add style, "position: relative;" to the popover container to help with positioning flow. Dropdown and DatePicker had "relative" prop that would be passed down to child popover component. Code was adjusted to always set style, "position: relative;" and to calculate the position of popover when displaying upwards.

After (the change)

Popover always has "position: relative" (related to #2003):

2025-01-20-popover-position-relative.mp4

Popover positioning is still okay when scrolling within a modal (related to #2466):

2025-03-12-popover-in-scrollable-modal-fix.mp4

Note: a popover cannot appear over top of modal because of z-index layer hierarchy

  • E.g. basic or filterable Dropdown (E.g. <goa-dropdown filterable="true">) cannot have its contents "drop down" outside of modal like a native Dropdown can, because native html select is not affected by z-index layering and will always show over top
  • this was already the behavior of popover when relative property was used before these changes (E.g. <goa-dropdown relative="true">)

Make sure that you've checked the boxes below before you submit the PR

  • I have read and followed the setup steps
  • I have created necessary unit tests
  • I have tested the functionality in both React and Angular.

Steps needed to test

Try dropdown and datepicker, some with a wrapper that has "position: relative" and some inside a modal -- try resizing the viewport to see how it calculates space below to determine if it dropdown or datepicker should expand upwards -- and try this in the different playgrounds:

@BumbleB2na BumbleB2na linked an issue Jan 20, 2025 that may be closed by this pull request
@BumbleB2na BumbleB2na force-pushed the 2003-popover-remove-relative-prop branch 2 times, most recently from 2205da4 to 961eb31 Compare January 20, 2025 22:48
@ArakTaiRoth
Copy link
Collaborator

This PR needs to be rebased with alpha after #2344 is merged in, and then needs to be re-tested

@BumbleB2na BumbleB2na force-pushed the 2003-popover-remove-relative-prop branch from 961eb31 to 1e25ca8 Compare February 11, 2025 23:33
@BumbleB2na BumbleB2na force-pushed the 2003-popover-remove-relative-prop branch from 1e25ca8 to 8b24789 Compare February 27, 2025 22:14
@BumbleB2na BumbleB2na force-pushed the 2003-popover-remove-relative-prop branch from 8b24789 to d9ad6ad Compare March 11, 2025 20:16
@BumbleB2na BumbleB2na linked an issue Mar 12, 2025 that may be closed by this pull request
@BumbleB2na
Copy link
Collaborator Author

This PR needs to be rebased with alpha after #2344 is merged in, and then needs to be re-tested

This is done. Then linked a second issue, #2466 and updated this PR description with a second video to demonstrate how that got resolved.

@ArakTaiRoth
Copy link
Collaborator

@vanessatran-ddi When testing this, can you also verify that everything works on a page, as well as within a modal?

@vanessatran-ddi
Copy link
Collaborator

@vanessatran-ddi When testing this, can you also verify that everything works on a page, as well as within a modal?

@ArakTaiRoth Yes I will do

@BumbleB2na BumbleB2na force-pushed the 2003-popover-remove-relative-prop branch from d1aafad to 6d0e4a0 Compare April 7, 2025 15:15
@BumbleB2na BumbleB2na marked this pull request as ready for review April 7, 2025 15:16
@BumbleB2na BumbleB2na force-pushed the 2003-popover-remove-relative-prop branch from 6d0e4a0 to 07bf4d8 Compare April 8, 2025 23:06
@BumbleB2na
Copy link
Collaborator Author

Updated PR to deprecate relative property instead of removing it. However, the property no longer has any effect (every popover now has postion: relative by default) and developers will see a console warning if using either of the 3 affected components:

image

@BumbleB2na BumbleB2na force-pushed the 2003-popover-remove-relative-prop branch from 07bf4d8 to a6b776e Compare April 8, 2025 23:22
@minhthytran
Copy link
Collaborator

minhthytran commented Apr 9, 2025

Issue #2466 is fixed on this PR. Has been tested with:

  • new angular
  • new react
  • firefox
  • chrome (mac) and Edge (windows)
  • safari
  • safari/chrome on iOS

image

Issue #2003 is fixed.
image

I tested with:

  • Dropdown (angular new, react new) has warning message.

@BumbleB2na I recommended you to add jsdocs with deprecated such as below:

image (Instead of add a comment next to it)

So the IDE can help developers to understand that the property can still be used but should be removed.
image

@dustin-nielsen-goa
Copy link
Collaborator

dustin-nielsen-goa commented Apr 9, 2025

@BumbleB2na There's a problem using the Dropdown in the AppHeaderMenu component, it appears that the width isn't being calculated properly
Before:
before

After:
after

@BumbleB2na BumbleB2na force-pushed the 2003-popover-remove-relative-prop branch 2 times, most recently from 6ec5938 to 2f234f1 Compare April 14, 2025 19:59
@BumbleB2na
Copy link
Collaborator Author

@BumbleB2na There's a problem using the Dropdown in the AppHeaderMenu component, it appears that the width isn't being calculated properly

Was not able to reproduce width problem but, this aided in the discovery that AppHeader and AppHeaderMenu depended on popover not having position: relative so, these two components override that style to turn it off, allowing them to display the same as before.

@BumbleB2na BumbleB2na requested review from dustin-nielsen-goa and removed request for syedszeeshan and vanessatran-ddi April 14, 2025 20:07
@BumbleB2na BumbleB2na force-pushed the 2003-popover-remove-relative-prop branch from 2f234f1 to bb3e555 Compare April 16, 2025 22:02
@ArakTaiRoth
Copy link
Collaborator

Tested and looks good now

@chrisolsen
Copy link
Collaborator

I am seeing the following

  1. When a non-native dropdown is at the bottom of a modal (this was the reason for previously having the relative prop)
    image

  2. When wrapping the dropdown within an absolutely positioned parent element (the calendar belongs to the left most element)
    image

Copy link
Collaborator

@chrisolsen chrisolsen left a comment

Choose a reason for hiding this comment

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

See my most recent comment

@BumbleB2na
Copy link
Collaborator Author

I am seeing the following

  1. When a non-native dropdown is at the bottom of a modal (this was the reason for previously having the relative prop)

Yes when the dropdown expands down at bottom of modal it increases the modal contents height. Your screenshot doesn't show the entire modal but, with this fix that makes popover part of the same z-index layering as modal, it's normal to have to scroll down:
image

I've created a new follow up issue, that depends on this PR, to take care of the issue you're seeing: #2655

  1. When wrapping the [date picker[ within an absolutely positioned parent element (the calendar belongs to the left most element)

Please provide a code example because, I could not reproduce the same problem:
image

@chrisolsen
Copy link
Collaborator

@BumbleB2na

  • I never noticed the scroll 👍
  • As for the position: absolute, the first test I made was to make it right: 0, which does result with the popover off the screen, but since using right is very much an edge case I removed the right attribute and saw the image that I posted after the screen auto-update, but I don't see it if I manually reload the page. So it everything now looks good to me.

@chrisolsen chrisolsen self-requested a review April 23, 2025 16:26
@chrisolsen chrisolsen merged commit 89ee3f2 into GovAlta:alpha Apr 23, 2025
1 check passed
@tzuge
Copy link
Collaborator

tzuge commented Apr 23, 2025

🎉 This PR is included in version 4.2.2-alpha.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tzuge
Copy link
Collaborator

tzuge commented Apr 23, 2025

🎉 This PR is included in version 1.2.2-alpha.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tzuge tzuge added the released on @alpha Released into alpha. label Apr 23, 2025
@tzuge
Copy link
Collaborator

tzuge commented Apr 23, 2025

🎉 This PR is included in version 1.33.0-alpha.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tzuge
Copy link
Collaborator

tzuge commented Apr 23, 2025

🎉 This PR is included in version 6.2.2-alpha.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tzuge
Copy link
Collaborator

tzuge commented May 1, 2025

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tzuge
Copy link
Collaborator

tzuge commented May 1, 2025

🎉 This PR is included in version 4.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tzuge
Copy link
Collaborator

tzuge commented May 1, 2025

🎉 This PR is included in version 1.33.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tzuge tzuge added released Released into production. labels May 1, 2025
@tzuge
Copy link
Collaborator

tzuge commented May 1, 2025

🎉 This PR is included in version 6.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released on @alpha Released into alpha. released Released into production.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Popover position in scrollable modal Popover: Remove the need for relative prop

7 participants