feat(#662): replace tooltips in maps with dialog#737
Conversation
🦋 Changeset detectedLatest commit: 7b9be23 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@garethbowen Could you please review when you have a chance? Tomorrow, I will make it available on a test server so Aly can share her feedback too. |
garethbowen
left a comment
garethbowen
left a comment
There was a problem hiding this comment.
Works well for me (Firefox desktop).
A few suggestions inline...
| --odk-spacing-m: 10px; | ||
| --odk-spacing-l: 15px; | ||
| --odk-spacing-xl: 20px; | ||
| --odk-spacing-xxl: 30px; |
There was a problem hiding this comment.
Ooh I need this one! :)
| } | ||
|
|
||
| @mixin map-block-sm { | ||
| @container map-block (max-height: 333px) { |
There was a problem hiding this comment.
CSS doesn't support CSS variables in media or container queries. Instead of adding SASS variables, we try to avoid doing so. I considered using mixins instead.
There was a problem hiding this comment.
If you're on board with using mixins for media or container queries, I'd be happy to open a follow-up PR for that small refactor :)
3 participants
Closes #662
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
Select one from map
Geopoint with maps appearance
Geotrace
Phone lanscape - info icon is hidden to not cover zoom buttons
Why is this the best possible solution? Were any other approaches considered?
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Do we need any specific form for testing your changes? If so, please attach one.
What's changed