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

fix contextmenu wrapping and submenu displaying bugs #732

Merged
merged 5 commits into from
Jan 31, 2023

Conversation

yhy0217
Copy link
Contributor

@yhy0217 yhy0217 commented Jan 27, 2023

Closes #684

  • fix the context menu wrapping bug
  • stop the context menu and submenu from going outside from the map when clicking at margin
  • fix the bug that submenu keeps displaying when moving mouse out from 'Copy Coordinates (C)' button in context menu
  • fix the submenu cutting off issue when the whole menu is opened near the bottom of the page
  • create a test in mapContextMenu.test.js

@AliyanH
Copy link
Member

AliyanH commented Jan 30, 2023

@yhy0217 found a small bug, when I open the layer context menu, the map contextmenu opens also:

image

Also, noticed this when opening the context menu for the layer near the margin, if its a simple fix, are you able to replicate the same function for the layer contextmenu as you did for the map contextmenu:

image

If that's not as simple, you can create a new issue and we can handle it in a separate PR.

@yhy0217
Copy link
Contributor Author

yhy0217 commented Jan 30, 2023

@yhy0217 found a small bug, when I open the layer context menu, the map contextmenu opens also:

image

Also, noticed this when opening the context menu for the layer near the margin, if its a simple fix, are you able to replicate the same function for the layer contextmenu as you did for the map contextmenu:

image

If that's not as simple, you can create a new issue and we can handle it in a separate PR.

Finished to fix the bug that two menus open at the same time; however the layer menu does not wrap up on my laptop, and I think it should not happen because the new codes change the style of all buttons inside both two menus (as they all have same classname ".mapml-contextmenu-item"), but I may be wrong. Could you please run the code again and see if there is any change about the styling?

@AliyanH
Copy link
Member

AliyanH commented Jan 30, 2023

Finished to fix the bug that two menus open at the same time

Nice!

Could you please run the code again and see if there is any change about the styling?

Yup, the wrapping issue was solved, I may not have had the latest CSS.

Good work, this is looking great, the contextmenu is appearing as it should. Just found one other thing when the contextmenu is opened near the bottom of the map (the coordinates menu cuts out). Other than that, its perfect, really good work on this!
image

@yhy0217
Copy link
Contributor Author

yhy0217 commented Jan 31, 2023

Just found one other thing when the contextmenu is opened near the bottom of the map (the coordinates menu cuts out).

Yes, it used to look like this, but I fixed this problem in my first commit, and it now works well on my laptop:

Picture1

So I think a re-run may solve this issue too! :)

@AliyanH
Copy link
Member

AliyanH commented Jan 31, 2023

So I think a re-run may solve this issue too!

I believe this is different, this happens when the right click is initiated from the red spot shown below:
image

@yhy0217
Copy link
Contributor Author

yhy0217 commented Jan 31, 2023

I believe this is different, this happens when the right click is initiated from the red spot shown below:

Yes you are right, sorry for my misunderstanding. Have updated the code to fix it

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.

Looks good to me! Changing the magic numbers to variables would make the code more meaningful, which you have mostly done.

@prushforth prushforth marked this pull request as ready for review January 31, 2023 16:53
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.

LGTM!

@prushforth
Copy link
Member

LGTM, please merge / rebase and merge when ready.

@yhy0217 yhy0217 merged commit f101823 into Maps4HTML:main Jan 31, 2023
@AliyanH AliyanH mentioned this pull request Feb 8, 2023
6 tasks
@yhy0217 yhy0217 deleted the contextMenuWrapping branch February 14, 2023 13:53
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.

Menu items in the context menu shouldn't wrap
3 participants