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.

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

Add contextmenu for map-extent layercontrol #930

Merged
merged 7 commits into from
Mar 15, 2024

Conversation

AliyanH
Copy link
Member

@AliyanH AliyanH commented Mar 10, 2024

Closes #926

  • Add "Zoom to Sub Layer" + "Copy Sub Layer"
  • Add support for keyboard navigation
  • Add support for Keyboard shortcut
  • Add localization
  • Add missing localization for "Sub-Layer"
  • Re-key copy layer/extent shortcut from 'L' to 'C'?
  • when layer control context menu is open DO NOT collapse layer control?
  • Add tests

Future work

  • Abiltiy to paste map-extent into layers?

@prushforth prushforth self-requested a review March 12, 2024 19:29
Copy link
Member

@prushforth prushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to merge, integrate into release 0.13.1

@@ -71,6 +71,45 @@ export class MapExtent extends HTMLElement {
? getExtent(this)
: getCalculatedExtent(this);
}

getOuterHTML() {
let tempElement = this.cloneNode(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I attempted, but failed to re-write this without cloning the live element. It's that cloning that yields the console logging due to disconnected map-input and so on (I believe). That logging seems to interfere with the tests, and what's concerning is that it doesn't appear to happen consistently.

@@ -107,7 +146,7 @@ export class MapExtent extends HTMLElement {
case 'label':
if (oldValue !== newValue) {
this._layerControlHTML.querySelector(
'.mapml-layer-item-name'
'.mapml-extent-item-name'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good going

@@ -16,8 +16,8 @@ test.describe('Using arrow keys to navigate context menu', () => {

test('Testing layer contextmenu', async () => {
await page.waitForTimeout(500);
await page.click('body > mapml-viewer');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not recommended by playwright, so I changed a few of them where they seemed to cause some flakiness

@AliyanH
Copy link
Member Author

AliyanH commented Mar 12, 2024

Thanks @prushforth! Will merge after finalizing Maps4HTML/mapml-extension#69

@prushforth
Copy link
Member

Thanks @prushforth! Will merge after finalizing Maps4HTML/mapml-extension#69

We should merge this when it's ready so that we can release it as 0.13.1 (with a small other GeoServer-related change that wouldn't pass CI reliably without these commits) with GeoServer 2.25.0, which will happen March 18th. I could merge that change without this, of course, but it was easier to rebase it on the work done in this branch which eliminates some test flakiness (there's always a new source of flakiness, for some reason).

The GeoServer viewer should not depend on the extension, but I agree the extension needs this update.

I will open a GeoServer PR this week to include the new viewer, which we can update until Friday.

@AliyanH AliyanH merged commit 7817c6c into Maps4HTML:main Mar 15, 2024
1 check passed
@prushforth
Copy link
Member

I'll cut a 0.13.1 release later today, hopefully in time to make the GeoServer 2.25.0 release cut-off, tbd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants