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

full screen context menu item added and fixed some context menu related tests #765

Merged
merged 21 commits into from
Mar 1, 2023

Conversation

Jacky0299
Copy link
Contributor

@Jacky0299 Jacky0299 commented Feb 21, 2023

New "Exit Fullscreen" locale added to the mapml-extension here: Maps4HTML/mapml-extension@c446c54

solves issue #201

@AliyanH

This comment was marked as resolved.

@Jacky0299 Jacky0299 marked this pull request as ready for review February 22, 2023 21:14
@prushforth

This comment was marked as resolved.

src/mapml-viewer.js Outdated Show resolved Hide resolved
src/mapml/control/FullscreenButton.js Outdated Show resolved Hide resolved
src/mapml/control/FullscreenButton.js Outdated Show resolved Hide resolved
@prushforth
Copy link
Member

A good place to start reading about the browsers' Fullscreen API is here; there are likely things in there that would be useful in the code for this PR.

@AliyanH
Copy link
Member

AliyanH commented Feb 27, 2023

Found a weird behavior when navigating the history using alt+Arrow keys, when I go back in history many times using the alt+back arrow key, the forward history seems to get deleted, as seen below:

Video.mp4

Probably because the alt+arrowkeys are still doing something when at the end of history, when they are at the end of history it should just prevent default and not be going forward/backwards - that might be the cause of this.

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.

A couple of small clarifying changes

src/mapml/handlers/ContextMenu.js Outdated Show resolved Hide resolved
src/mapml/handlers/ContextMenu.js Outdated Show resolved Hide resolved
src/web-map.js Outdated Show resolved Hide resolved
@prushforth
Copy link
Member

Found a weird behavior when navigating the history using alt+Arrow keys, when I go back in history many times using the alt+back arrow key, the forward history seems to get deleted, as seen below:

Probably because the alt+arrowkeys are still doing something when at the end of history, when they are at the end of history it should just prevent default and not be going forward/backwards - that might be the cause of this.

Is this on web-map or mapml-viewer? Not sure if it makes a difference, but I noticed that alt+left-arrow is enabled on any div, not just the map div.

@Jacky0299
Copy link
Contributor Author

Found a weird behavior when navigating the history using alt+Arrow keys, when I go back in history many times using the alt+back arrow key, the forward history seems to get deleted, as seen below:

Probably because the alt+arrowkeys are still doing something when at the end of history, when they are at the end of history it should just prevent default and not be going forward/backwards - that might be the cause of this.

I think it is because pressing it too fast, if you press it once by once, it's working fine, I can try to put a timeout between each time back() and forward() triggers.

@prushforth
Copy link
Member

prushforth commented Feb 27, 2023

I noticed that if you use "Alt+Left-Arrow" until there is no more history, the "Reload" button does not become disabled, until you actually reload / Ctrl-R. That is, when you pop the last history item, it should disable the reload button.

@Jacky0299
Copy link
Contributor Author

I noticed that if you use "Alt+Left-Arrow" until there is no more history, the "Reload" button does not become disabled, until you actually reload / Ctrl-R. That is, when you pop the last history item, it should disable the reload button.

I don't really see this problem at my side, when there's no more history, the reload is disabled. It should be the same as before since I did not really touch the functions.

Copy link
Member

@AliyanH AliyanH left a comment

Choose a reason for hiding this comment

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

code structure change

src/mapml-viewer.js Outdated Show resolved Hide resolved
@AliyanH
Copy link
Member

AliyanH commented Feb 28, 2023

I think we can revert the settimeout change and look at async await if possible.

With the settimeout, we are only able to go back with intervals of 0.5 seconds. So If I were to go back twice within 0.5 secs, It would only go back once:

Video.mp4

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.

A few comments. Almost there!

src/mapml-viewer.js Outdated Show resolved Hide resolved
src/mapml-viewer.js Outdated Show resolved Hide resolved
src/mapml-viewer.js Show resolved Hide resolved
src/mapml-viewer.js Outdated Show resolved Hide resolved
src/mapml-viewer.js Outdated Show resolved Hide resolved
this._map.contextMenu._items[0].el.el.disabled = false; // back contextmenu item
this._map.contextMenu._items[2].el.el.disabled = false; // reload contextmenu item
this._map.contextMenu.toggleContextMenuItems("Back", "enabled"); // back contextmenu item
this._map.contextMenu.toggleContextMenuItems("Reload", "enabled"); // reload contextmenu item
Copy link
Member

Choose a reason for hiding this comment

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

Here is where you should also enable the reload button on the map.

this._map.contextMenu._items[2].el.el.disabled = true; // reload contextmenu item
this._map.contextMenu.toggleContextMenuItems("Back", "disabled"); // back contextmenu item
this._map.contextMenu.toggleContextMenuItems("Forward", "disabled");// forward contextmenu item
this._map.contextMenu.toggleContextMenuItems("Reload", "disabled"); // reload contextmenu item
Copy link
Member

Choose a reason for hiding this comment

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

Here is where you should also disable the reload button on the map

src/mapml/handlers/ContextMenu.js Outdated Show resolved Hide resolved
src/mapml/handlers/ContextMenu.js Outdated Show resolved Hide resolved
src/mapml/handlers/ContextMenu.js Outdated Show resolved Hide resolved
@prushforth
Copy link
Member

@Jacky0299 I pushed some changes (pull them and have a look!) that maybe weren't clear; I left the comments above so you can compare what I did with what the comment asked for.

@prushforth
Copy link
Member

@Jacky0299 Squash and merge when ready.

Comment on lines +20 to +24
text: M.options.locale.cmBack + " (<kbd>Alt+Left Arrow</kbd>)",
callback:this._goBack
},
{
text: M.options.locale.cmForward + " (<kbd>F</kbd>)",
text: M.options.locale.cmForward + " (<kbd>Alt+Right Arrow</kbd>)",
Copy link
Member

Choose a reason for hiding this comment

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

Looks good, just dont forget to add localized versions of this in the extension, similar to what you did for "Exit Fullscreen". Create a PR for the extension.

Nice Work on this!

@Jacky0299 Jacky0299 merged commit d89e27b into Maps4HTML:main Mar 1, 2023
prushforth added a commit to prushforth/MapML.js that referenced this pull request Mar 6, 2023
…ed tests (Maps4HTML#765)

* full screen context menu item added and fixedtests

* weird test: Checking Context Menu Fullscreen

* ready for review

* Fix fullscreen test and remove scrolling

* minor changes

* ready to merge

* bug fix (esc button), Peter suggested functions

* forward reload back fullscreen button changes

* minor changes

* Linting

* Change "View fullscreen" to "View Fullscreen"

* minor linting

* Aliyan suggested styling changes

* time out added between each time back is pressed

* disabling reload button

* bugs fix and new context menu functions

* sync changes with web map

* peter suggested changes

* Tweak to mapml-viewer, web-map

---------

Co-authored-by: AliyanH <aliyanulhaq@gmail.com>
Co-authored-by: Peter Rushforth <peter.rushforth@gmail.com>
Co-authored-by: prushfor <prushfor@L-BSC-A146390.nrn.nrcan.gc.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants