-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Extra control placement 3039 #13104
base: main
Are you sure you want to change the base?
Extra control placement 3039 #13104
Conversation
…+ top and bottom. Working with some bugs (e.g scale control + any other control causes other control to distort)
…oes clockwise starting from top left until center left
Sorry for the late response — this PR is on our mind and we'll look at it closer after we're done with the current v3.3 release. 🙏 |
No problem, thanks for the update! |
src/ui/map.js
Outdated
@@ -83,7 +83,7 @@ import type {ContextOptions} from '../gl/context.js'; | |||
import * as TP from '../tracked-parameters/tracked_parameters.js'; | |||
import * as TPM from '../tracked-parameters/tracked_parameters_mock.js'; | |||
|
|||
export type ControlPosition = 'top-left' | 'top-right' | 'bottom-left' | 'bottom-right'; | |||
export type ControlPosition = 'top-left' | 'top-center' | 'top-right' | 'right-center' | 'bottom-right' | 'bottom-center' | 'bottom-left' | 'left-center'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like left-center
and right-center
are flipped relative to the others. Unless there's good precedent elsewhere, would putting the vertical positioning consistently first be easier to remember?
PR suggestion:
top-left | top-center | top-right |
left-center | right-center | |
bottom-left | bottom-center | bottom-right |
Maybe easier to remember:
top-left | top-center | top-right |
center-left | center-right | |
bottom-left | bottom-center | bottom-right |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit of a tossup because I wanted to avoid having "center" show up as both the first and last word. Maybe make "bottom-center" into "center-bottom" too (as well as with "top-center")? Or maybe make those two "bottom-middle" and "top-middle"?
Suggestion 1 | ||
---|---|---|
top-left | center-top | top-right |
center-left | center-right | |
bottom-left | center-bottom | bottom-right |
Suggestion 2 | ||
---|---|---|
top-left | top-middle | top-right |
center-left | center-right | |
bottom-left | bottom-middle | bottom-right |
I don't know which is best, seems like there are only tradeoffs unless we bite the bullet with my suggestion 2 and have "middle" and "center" refer to different axes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I know I'm engaging in some drive-by nitpicking; it just reminds me of properties I have to look up every single time, e.g. flexbox alignment vs. justification and SVG alignment-baseline center vs. central vs. middle. So I don't have a strong stance, just that some amount of logic, precedent, or memorability would be a nice goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I like this, but what about:
Suggestion 3 | ||
---|---|---|
top-left | top | top-right |
left | right | |
bottom-left | bottom | bottom-right |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could work and I'm leaning towards it the most. Thanks for this suggestion
This would be sick to get out |
It seems that no changes since last comment by @Ethan-Guttman. |
I was waiting for a contact from the Mapbox team before updating again (since they said they would look at it closer later). I'll make the changes myself |
… (from center-left, center-right, center-top and center-bottom). This brings them more in line with other references to those positions throughout the code.
This closes the good first issue #3039 (Allow controls to be placed at top-center, bottom-center, left-center, and right-center)
example pictures showcasing all four new positions:
![image](https://private-user-images.githubusercontent.com/61851595/309403222-82b4929c-6dc4-4b7c-95a2-ae829f2abe2a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU2MDA3OTMsIm5iZiI6MTcyNTYwMDQ5MywicGF0aCI6Ii82MTg1MTU5NS8zMDk0MDMyMjItODJiNDkyOWMtNmRjNC00YjdjLTk1YTItYWU4MjlmMmFiZTJhLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA2VDA1MjgxM1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTRkNGNiZjAwZDllZjUxYzc5MjUyN2UzNWE0OWRiMTgwNGFjOGY0ZDEwZjllNTg5ZTU2MjlhZTQ5OTI3ZjM1NjYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.WUogsRMkCTiMoDcMEyftWa5BR9qtqaKfG6-58r7BYOU)
![image](https://private-user-images.githubusercontent.com/61851595/309403695-3cb4feba-c28f-451f-b0d3-46111b6219f7.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU2MDA3OTMsIm5iZiI6MTcyNTYwMDQ5MywicGF0aCI6Ii82MTg1MTU5NS8zMDk0MDM2OTUtM2NiNGZlYmEtYzI4Zi00NTFmLWIwZDMtNDYxMTFiNjIxOWY3LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA2VDA1MjgxM1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWRlYjAzMWViOWQ5Y2Q0Njk3M2JkOWQ3MGE1OTQxZjI2MWRkMzg5N2FhNmViZTMzYmM4YjhkOWQzMDFlOWFmMzAmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.nJ5KpfAlEJS93ZwshkBIkxTnoa5SkTbSvVHyWT7RSTk)
Multiple ctrls will have margins between themselves like the already existing positions. However, depending on their widths in the top and bottom center they can look weird (smaller elements floating left).
I ordered the positions code in the css and map.js files by going from top-left to center-left clockwise listing each position too.