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

ui-manchette: fix waypoint menu positioning #668

Merged
merged 3 commits into from
Dec 5, 2024
Merged

ui-manchette: fix waypoint menu positioning #668

merged 3 commits into from
Dec 5, 2024

Conversation

SharglutDev
Copy link
Contributor

@SharglutDev SharglutDev commented Oct 24, 2024 •
edited
Loading

  • Move the menu next to its related waypoint so it can always be positioned relatively to it
  • To keep osrd-ui as neutral as possible, the menu has no styles, they need to be passed by props

fix #667

After discussion with @thibautsailly, it has been decided to totally block the scroll in the manchette (and only in the manchette) as long as the menu is displayed. If we still try to scroll, the whole page will scroll.

@SharglutDev SharglutDev requested a review from a team as a code owner October 24, 2024 17:37
@SharglutDev SharglutDev force-pushed the ui-manchette/fix-waypoint-menu branch from 85faa8f to c343ac1 Compare October 24, 2024 17:42
emersion
emersion reviewed Oct 25, 2024
View reviewed changes
ui-manchette/src/types.ts Outdated Show resolved Hide resolved
emersion
emersion reviewed Oct 25, 2024
View reviewed changes
ui-manchette/src/types.ts Outdated Show resolved Hide resolved
@emersion emersion mentioned this pull request Oct 25, 2024
Merged
@SharglutDev SharglutDev force-pushed the ui-manchette/fix-waypoint-menu branch 2 times, most recently from 54d11da to f725c80 Compare October 25, 2024 15:23
emersion
emersion reviewed Oct 25, 2024
View reviewed changes
ui-manchette/src/types.ts Outdated Show resolved Hide resolved
emersion
emersion approved these changes Oct 25, 2024
View reviewed changes
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

LGTM apart from this last question!

@SharglutDev SharglutDev force-pushed the ui-manchette/fix-waypoint-menu branch 3 times, most recently from 63703d6 to 1323383 Compare November 6, 2024 17:20
@SharglutDev SharglutDev force-pushed the ui-manchette/fix-waypoint-menu branch from 1323383 to b0f1076 Compare November 15, 2024 16:50
@SharglutDev SharglutDev requested a review from emersion November 15, 2024 16:51
emersion
emersion reviewed Nov 19, 2024
View reviewed changes
ui-manchette/src/types.ts Outdated Show resolved Hide resolved
emersion
emersion reviewed Nov 19, 2024
View reviewed changes
ui-manchette/src/types.ts Outdated Show resolved Hide resolved
@SharglutDev SharglutDev force-pushed the ui-manchette/fix-waypoint-menu branch from 390a28e to 99efc58 Compare November 25, 2024 11:23
@SharglutDev SharglutDev requested a review from emersion November 25, 2024 11:23
@SharglutDev SharglutDev force-pushed the ui-manchette/fix-waypoint-menu branch 2 times, most recently from bac18b8 to dd537cc Compare November 27, 2024 08:16
@SharglutDev SharglutDev requested a review from emersion November 27, 2024 08:18
@SharglutDev SharglutDev force-pushed the ui-manchette/fix-waypoint-menu branch 4 times, most recently from ad471d1 to d3c1306 Compare November 28, 2024 10:14
anisometropie
anisometropie requested changes Nov 28, 2024
View reviewed changes
ui-manchette/src/hooks/useElementInView.ts Outdated Show resolved Hide resolved
ui-manchette/src/hooks/useElementInView.ts Outdated Show resolved Hide resolved
ui-manchette/src/hooks/useElementInView.ts Outdated Show resolved Hide resolved
anisometropie
anisometropie requested changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@anisometropie anisometropie left a comment

Choose a reason for hiding this comment

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

the behavior is not consistent.
open the menu, then scroll down, and click and the menu shows on top of the waypoint the next time
https://github.com/user-attachments/assets/d5762a6b-62e8-4454-8fe9-455157bdc641

@SharglutDev
Copy link
Contributor Author

After discussion with @thibautsailly, it has been decided to totally block the scroll in the manchette (and only in the manchette) as long as the menu is displayed. If we still try to scroll, the whole page will scroll. (reported in the PR and issue description)

@emersion
Copy link
Member

emersion commented Dec 3, 2024 •
edited
Loading

This approach still sounds pretty fragile to me.

  • If we really want to block interactions while the menu is opened, I'd suggest using a portal to insert a fullscreen transparent overlay preventing all interactions except for the menu, and positioning the menu inside that overlay.
  • If we want to allow interactions while the menu is opened, I'd suggest we insert the menu outside of the manchette via a portal, then use requestAnimationFrame to update the position of the menu dynamically if the waypoint is visible. Alternatively, give a library such as floating-ui a shot.

@SharglutDev SharglutDev requested a review from emersion December 4, 2024 19:54
emersion
emersion reviewed Dec 5, 2024
View reviewed changes
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

We're getting close! A few new comments.

ui-manchette/src/components/Manchette.tsx Outdated Show resolved Hide resolved
ui-manchette/src/components/Manchette.tsx Outdated Show resolved Hide resolved
ui-manchette/src/components/Manchette.tsx Outdated Show resolved Hide resolved
ui-manchette/src/types.ts Outdated Show resolved Hide resolved
ui-manchette/src/components/Manchette.tsx Outdated Show resolved Hide resolved
emersion
emersion approved these changes Dec 5, 2024
View reviewed changes
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for your perseverance through this odyssey!

@SharglutDev SharglutDev force-pushed the ui-manchette/fix-waypoint-menu branch from 1b68e3e to 4542460 Compare December 5, 2024 17:26
@SharglutDev
ui-manchette: update some package exports
e7d2416 
- Add some type export needed when implementing the app
- Remove some exports not needed outside of ui-manchette

Signed-off-by: SharglutDev <[email protected]>
@SharglutDev
ui-manchette: fix manchette toolbar not fixed bottom
a9798c6 
The PR #728 changed the height of the manchette from a min height to a fixed one.
It introduced a bug where, when having a long list of waypoint, when scrolling bottom, the manchette toolbar wasn't stick to the bottom anymore.
Fix this issue by giving back a min height to the manchette while still keeping its dynamic value introduced in PR #728.

Signed-off-by: SharglutDev <[email protected]>
@SharglutDev
ui-manchette: fix waypoint menu positioning
795b7bc 
- Move the menu next to its related waypoint so it can always be positioned relatively to it
- To keep osrd-ui as neutral as possible, the menu has no styles, they need to be passed by props
- Add a custom hook using the intersection observer api. This will allow the menu to hide when its waypoint is hidden on scroll
- This hook will remain in osrd-ui to prevent a user to have the implement this necessary logic when adding a waypoint menu to the manchette

Signed-off-by: SharglutDev <[email protected]>
@SharglutDev SharglutDev force-pushed the ui-manchette/fix-waypoint-menu branch from 4542460 to 795b7bc Compare December 5, 2024 17:34
anisometropie
anisometropie approved these changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@anisometropie anisometropie left a comment

Choose a reason for hiding this comment

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

tested again, Good job !

anisometropie
anisometropie reviewed Dec 5, 2024
View reviewed changes
Copy link
Contributor

@anisometropie anisometropie left a comment

Choose a reason for hiding this comment

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

just a bit unsure about disabling completely all the interactions outside of the menu.
it’s pretty natural to close the menu by clicking outside of it

@SharglutDev
Copy link
Contributor Author

just a bit unsure about disabling completely all the interactions outside of the menu. it’s pretty natural to close the menu by clicking outside of it

It's just the story working like that. In osrd, you will be able to close the menu when clicking outside of it of course.

@SharglutDev SharglutDev added this pull request to the merge queue Dec 5, 2024
Merged via the queue into dev with commit f3bb01f Dec 5, 2024
6 checks passed
@SharglutDev SharglutDev deleted the ui-manchette/fix-waypoint-menu branch December 5, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anisometropie anisometropie anisometropie approved these changes

@emersion emersion emersion 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.

Waypoint menu position not dynamic
3 participants
@SharglutDev @emersion @anisometropie