feat: add context menu#745
Conversation
Also, remove unnecessary props variables.
Also replaces previous KernBlockButton.ce.vue implementation.
|
By using "role" the semantic list elements are no longer needed.
# Conflicts: # src/components/kern/KernBlockButtonCheckbox.ce.vue # src/components/kern/KernBlockButtonRadioGroup.ce.vue # src/core/types/moveHandle.ts
# Conflicts: # src/core/components/PolarContainer.ce.vue # src/core/stores/main.ts
# Conflicts: # src/core/components/PolarContainer.ce.vue
|
dibs |
| &:focus { | ||
| outline: auto; | ||
| } | ||
| } |
There was a problem hiding this comment.
We shouldn't initially display the focus on right-click, as it's confusing to some users. I've checked what Windows does and what other component libraries do, and they either have an "invisible initial" element at first that holds the focus so that pressing down goes to the first one, or have the first element selected, but not visibly so.
The latter would be the difference between :focus and :focus-visible, which I feel is preferable to the current state and easy to implement.
There was a problem hiding this comment.
So having the focus on the first element, but not having the focus visible here, is preferrable?
Testing the behaviour with multiple buttons can be achieved by adding the following snippet to the snowbox index.js
const buttons = [
{
id: 'color',
icon: 'kern-icon--palette',
text: 'Farben anpassen',
callback: (c) => console.warn('Colour', c),
},
{
id: 'path',
icon: 'kern-icon--rebase-edit',
text: 'Pfad anpassen',
callback: (c) => console.warn('Path', c),
},
{
id: 'duplicate',
icon: 'kern-icon--content-copy',
text: 'Duplizieren',
callback: (c) => console.warn('Duplicate', c),
},
{
id: 'delete',
icon: 'kern-icon--delete',
text: 'Löschen',
callback: (c) => console.warn('Delete', c),
color: 'oklch(50.78% 0.202 29.22)',
group: 'delete',
},
]
buttons.forEach((b) => map.store.addToContextMenu(b))There was a problem hiding this comment.
So having the focus on the first element, but not having the focus visible here, is preferrable?
Yeah, or, if you dislike that, there's the other option I pointed out.
However, :focus-visible is explicitly built for this.
https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/:focus-visible
It even uses a "variety of heuristics".
| justify-content: start; | ||
|
|
||
| &:focus { | ||
| outline: auto; |
There was a problem hiding this comment.
The outline does not match the button style.
With increased font size, you can easily see there's a small gap there in the top-left corner. It's also visible without increased font size, and the differing border-radius is also discernible anyway. Just posting this since it makes it very visible.
While we're here, the outline does use a different style than e.g. the icon menu. I'd prefer it if it used the same effects. Example for colour:
There was a problem hiding this comment.
The border-radius of the card now matches the buttons 7120ee5
The outline is the default outline of the browser. The outline of the icon buttons of the IconMenu use a completely different approach so that an active button can be highlighted as well.
There was a problem hiding this comment.
The outline of the icon buttons of the IconMenu use a completely different approach so that an active button can be highlighted as well.
Impressive. Very nice. Can you change it?
Since the outline is also pretty much on the map background, and I don't fancy multiple outline styles in one product, they should look the same.
There was a problem hiding this comment.
We can also make it an issue.
There was a problem hiding this comment.
What style should we be using?
KERN currently works with
outline: 4px solid var(--kern-color-action-focus-default-contextual, #454B6B);
outline-offset: 2px;This approach wouldn't really work with the IconMenu-Buttons. So should we be deviating completely and just use the styling we currently see on the PolarIconButton?
|
|
||
| mainStore.map | ||
| .getTargetElement() | ||
| .addEventListener('contextmenu', openContextMenu) |
There was a problem hiding this comment.
Here and Line findLineWith("// Suppresses the context menu of the browser"):
This effectively overrides the possibility to "normally" open the native context menus on a canvas, that is, however, pretty useful.
I've checked and pressing Shift+RightClick on Firefox circumvents this problem: https://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event (Somewhere down there in the initial chapter.)
I figure that this same behaviour could be used in all browsers with little implementation effort so that users can use the native context menu anyway. A small hint at the bottom of the context menu would be helpful, too. What do you think?
There was a problem hiding this comment.
That is intended and in line with how both Google Maps and OpenStreetMap handle their contextmenu.
I don't think that implementing browser specific ways to open the default contextmenu of the browser is something we should be doing.
There was a problem hiding this comment.
Since most browser don't implement it at all and we block the browser default behaviour, I think we should.
I sent an inquiry to our user experience human.
|
|
||
| function setupPlugin() { | ||
| coreStore.map.on('pointermove', updatePointerPosition) | ||
| coreStore.addToContextMenu({ |
There was a problem hiding this comment.
Do we need configurable context menus? Letting each plugin just do it directly is a solution, albeit not a very dynamic one. I'm also thinking about initialization order and how this affects the view. 🤔
However, this is a complex topic. My first idea was that core, after initialization of all plugins, calls getMenuEntries on all (in a new config.contextMenu) configured plugins, which is a new optional API endpoint ... but then the context menu would be a little static, since especially tools like draw will want to add and remove entries depending on their context. Bleh. Maybe simple ordering the groups in global config is enough.
What do you think about this? Is it a topic we should discuss now, or postpone to later?
There was a problem hiding this comment.
I say we postpone this. I even suggest we write an issue for this to tackle this after draw has been migrated.
# Conflicts: # src/core/types/moveHandle.ts # src/core/utils/map/setupMarkers.ts # src/plugins/pointerPosition/store.ts
|
Except #745 (comment), I've tackled every thread. That is something, I'll need some more time for. 🏓 @warm-coolguy until then |
…clarification of heritage
|

Summary
Adds the possibility for plugins to insert themselves in a context menu by calling
addToContextMenu. Entries can be removed by callingremoveFromContextMenu.Instructions for local reproduction and review
snowboxKnown issues
On mobile, a pin is still being set as OpenLayers already fires a
singleclick-event before the 1000ms of thelongPressHammerhave elapsed.Relevant tickets, issues, et cetera
Resolves: #403
Blocked by: #733, #734, #748