Merged
Conversation
Passing run #2276 ↗︎
Details:
Review all test suite changes for PR #815 ↗︎ |
|||||||||||||||
5495cd3 to
9f2a197
Compare
e491187 to
f6d2e44
Compare
9f2a197 to
45c748c
Compare
f6d2e44 to
c8d6a4b
Compare
45c748c to
5819e4b
Compare
c8d6a4b to
ceff221
Compare
f3b1953 to
8c621f8
Compare
ltkum
reviewed
May 7, 2024
Contributor
ltkum
left a comment
There was a problem hiding this comment.
looks pretty good to me. Quick question:
When switching from floating to fixed, then to floating again, it doesn't keep the previous position. Same with reloading the page.
Do we want, in the future, to allow people to have the floating tooltip position be something they can share ?
It seems to work and I don't think I've seen anything problematic in the code, but I would just make a quick e2e test to see if we can move the infobox.
8c621f8 to
ce27066
Compare
ltshb
reviewed
May 7, 2024
Contributor
ltshb
left a comment
There was a problem hiding this comment.
A quick first impression looks good, here already a few details. I'll do the full review later on.
ltshb
reviewed
May 7, 2024
d7370c5 to
f347afe
Compare
ltshb
added a commit
that referenced
this pull request
May 15, 2024
The issue with the modal is that the user loose its content when clicking outside of the modal ! Once the PR #815 is done we can make the window movable.
ltshb
added a commit
that referenced
this pull request
May 15, 2024
The issue with the modal is that the user loose its content when clicking outside of the modal ! Once the PR #815 is done we can make the window movable.
f347afe to
d0134d1
Compare
692289b to
c4c8871
Compare
ltkum
reviewed
May 17, 2024
Contributor
ltkum
left a comment
There was a problem hiding this comment.
A few things I noticed while using the tooltip :
- When moving around, the tooltip follows, but when reducing the size of the window, the tooltip stays where it was. This means that if we reduce the window enough to hide the tooltip, we no longer have a tooltip shown, even when clicking on new features.
- I had one instance where I wasn't able to drop the tooltip after dragging it. It lasted 5-6 seconds, and I think it's most certainly a local issue with my computer, but mentioning it here in the case someone else encounter the same issue and it turns out it's not a computer issue.
- It could be interesting (maybe more a post prod element) to try to position the tooltip the first time out of the selection. If someone selects a feature on the top right corner, we could move the tooltip slightly to show it, and if it's not possible (for example : someone selects the whole map), we simply put it in its default position
- Do we want to be able to remember the tooltip position ? Right now, if I press the 'move to footer' button by accident and want to get my tooltip back, I'll need to manually move it to the previous position after making it a floating tooltip again. And if I want to share a link to someone, I might be thinking the position of the tooltip is shared too. I think we can keep this idea for a post prod discussion.
ltshb
reviewed
May 21, 2024
Contributor
ltshb
left a comment
There was a problem hiding this comment.
Some small issues:
- Cursor when hovering the floating window title we should have the move cursor
- When hovering an item in the list of features we should have the pointer cursor showing that it is clickable (issue already present before)
- Show profile is not very intuitive and have several issue that we might want at best discuss offline and fix in separate PR.
48ba9ee to
806f325
Compare
and activate it by default on desktop, with a button to let the user go back to the "sticky to feature" mode
Swapping the infobox minimize/expand button to caret based icons, and add a minimize/expand button to map popover Removing the button to change the popup mode (attached to feature or floating) and only show the tooltip as floating (even in drawing mode)
Shuffle the popup button to have the print "at the border" (will be removed for go-live) Filling the infobox entirely even if there's only one "set" of features Share networks : find a way to have them in one line in location popup, using a grid box to wrap (if necessary) instead of a flexbox, more resilient to wrapping
Placing it depending on mobile/desktop drawing elements, and adding checks at tooltip mount to make sure it is within the given limits. Exporting a bunch of extra SCSS variable and gathering them all in a single JS file to ease their imports down the line. Adding a bit of padding while editing a feature in the infobox (UI was glued to the border of the screen otherwise...)
as we want to test a profile edit, a very narrow screen (small height) is very cluttered and cannot really be tester properly
ltshb
approved these changes
May 23, 2024
add a bit more padding to the edit feature in the infobox (matching the padding of the floating tooltip) force cursor to be a pointer over the feature list headers
806f325 to
24d7b1b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test link