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

Extra control placement 3039 #13104

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Extra control placement 3039 #13104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cbe2b05
Added control css options to set controls at the center of the sides …
Ethan-Guttman Feb 28, 2024
465af00
Changed convention so that each listing of different ctrl positions g…
Ethan-Guttman Feb 29, 2024
d95d740
More style changes to further support new ctrl positions
Ethan-Guttman Mar 1, 2024
7ee3756
Fixed other ctrl expanding with scale ctrl in bottom and top center p…
Ethan-Guttman Mar 1, 2024
8d396d3
fix lint error
Ethan-Guttman Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 34 additions & 8 deletions src/css/mapbox-gl.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,19 @@
.mapboxgl-ctrl-top-left,
.mapboxgl-ctrl-top-right,
.mapboxgl-ctrl-bottom-left,
.mapboxgl-ctrl-bottom-right { position: absolute; pointer-events: none; z-index: 2; }
.mapboxgl-ctrl-bottom-right,
.mapboxgl-ctrl-top-center,
.mapboxgl-ctrl-right-center,
.mapboxgl-ctrl-bottom-center,
.mapboxgl-ctrl-left-center { position: absolute; pointer-events: none; z-index: 2; }
.mapboxgl-ctrl-top-left { top: 0; left: 0; }
.mapboxgl-ctrl-top-center { top:0; left: 50%; transform: translateX(-50%); }
.mapboxgl-ctrl-top-right { top: 0; right: 0; }
.mapboxgl-ctrl-bottom-left { bottom: 0; left: 0; }
.mapboxgl-ctrl-right-center { top:50%; transform: translateY(-50%); right:0; }
.mapboxgl-ctrl-bottom-right { right: 0; bottom: 0; }
.mapboxgl-ctrl-bottom-center { bottom: 0; left: 50%; transform: translateX(-50%); }
.mapboxgl-ctrl-bottom-left { bottom: 0; left: 0; }
.mapboxgl-ctrl-left-center { top: 50%; transform: translateY(-50%); left: 0 }

.mapboxgl-ctrl {
clear: both;
Expand All @@ -67,10 +75,14 @@
/* workaround for a Safari bug https://github.com/mapbox/mapbox-gl-js/issues/8185 */
transform: translate(0, 0);
}
.mapboxgl-ctrl-top-left .mapboxgl-ctrl { margin: 10px 0 0 10px; float: left; }
.mapboxgl-ctrl-top-right .mapboxgl-ctrl { margin: 10px 10px 0 0; float: right; }
.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl { margin: 0 0 10px 10px; float: left; }
.mapboxgl-ctrl-bottom-right .mapboxgl-ctrl { margin: 0 10px 10px 0; float: right; }
.mapboxgl-ctrl-top-left .mapboxgl-ctrl { margin: 10px 0 0 10px; float: left; }
.mapboxgl-ctrl-top-center .mapboxgl-ctrl { margin: 10px 0; float: left;}
.mapboxgl-ctrl-top-right .mapboxgl-ctrl { margin: 10px 10px 0 0; float: right; }
.mapboxgl-ctrl-right-center .mapboxgl-ctrl { margin: 0 10px 10px 0; float: right; }
.mapboxgl-ctrl-bottom-right .mapboxgl-ctrl { margin: 0 10px 10px 0; float: right; }
.mapboxgl-ctrl-bottom-center .mapboxgl-ctrl { margin: 10px 0; float: left; }
.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl { margin: 0 0 10px 10px; float: left; }
.mapboxgl-ctrl-left-center .mapboxgl-ctrl { margin: 0 0 10px 10px; float: left;}

.mapboxgl-ctrl-group {
border-radius: 4px;
Expand Down Expand Up @@ -418,7 +430,8 @@ a.mapboxgl-ctrl-logo.mapboxgl-compact {
}

.mapboxgl-ctrl-top-left > .mapboxgl-ctrl-attrib.mapboxgl-compact-show,
.mapboxgl-ctrl-bottom-left > .mapboxgl-ctrl-attrib.mapboxgl-compact-show {
.mapboxgl-ctrl-bottom-left > .mapboxgl-ctrl-attrib.mapboxgl-compact-show,
.mapboxgl-ctrl-left-center > .mapboxgl-ctrl-attrib.mapboxgl-compact-show {
padding: 2px 8px 2px 28px;
border-radius: 12px;
}
Expand All @@ -444,7 +457,8 @@ a.mapboxgl-ctrl-logo.mapboxgl-compact {
}

.mapboxgl-ctrl-top-left .mapboxgl-ctrl-attrib-button,
.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl-attrib-button {
.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl-attrib-button,
.mapboxgl-ctrl-center-left .mapboxgl-ctrl-attrib-button {
left: 0;
}

Expand All @@ -462,11 +476,19 @@ a.mapboxgl-ctrl-logo.mapboxgl-compact {
right: 0;
}

.mapboxgl-ctrl-center-right > .mapboxgl-ctrl-attrib.mapboxgl-compact::after {
right: 0;
}

.mapboxgl-ctrl-top-right > .mapboxgl-ctrl-attrib.mapboxgl-compact::after {
top: 0;
right: 0;
}

.mapboxgl-ctrl-right-center > .mapboxgl-ctrl-attrib.mapboxgl-compact::after {
right: 0;
}

.mapboxgl-ctrl-top-left > .mapboxgl-ctrl-attrib.mapboxgl-compact::after {
top: 0;
left: 0;
Expand All @@ -476,6 +498,10 @@ a.mapboxgl-ctrl-logo.mapboxgl-compact {
bottom: 0;
left: 0;
}

.mapboxgl-ctrl-center-left > .mapboxgl-ctrl-attrib.mapboxgl-compact::after {
left: 0;
}
}

@media screen and (-ms-high-contrast: active) {
Expand Down
4 changes: 2 additions & 2 deletions src/ui/map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

@rreusser rreusser Apr 15, 2024 •
edited

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

Copy link
Author

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

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Author

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

/* eslint-disable no-use-before-define */
interface IControl {
+onAdd: (map: Map) => HTMLElement;
Expand Down Expand Up @@ -3396,7 +3396,7 @@ class Map extends Camera {

const controlContainer = this._controlContainer = DOM.create('div', 'mapboxgl-control-container', container);
const positions = this._controlPositions = {};
['top-left', 'top-right', 'bottom-left', 'bottom-right'].forEach((positionName) => {
['top-left', 'top-center', 'top-right', 'right-center', 'bottom-right', 'bottom-center', 'bottom-left', 'left-center'].forEach((positionName) => {
positions[positionName] = DOM.create('div', `mapboxgl-ctrl-${positionName}`, controlContainer);
});

Expand Down