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

New version of Map Navigation #5062

Merged
merged 62 commits into from Sep 7, 2021
Merged

Conversation

zoran995
Copy link
Contributor

@zoran995 zoran995 commented Dec 7, 2020

What this PR does

Requires TerriaJS/TerriaMap#505 also

Fixes #4338
Relates to #5020
Fixed some issues with the navigation menu on mobile with topElement, can't find them reported
Fixes part of #5765

when showing, you cannot click on the middle buttons (i.e. when trying to click on Compare, the Measuring Tool appears)

@AnaBelgun this is as mentioned in #4085 (comment) the way we implemented the new map navigation for terriajs

Sorry for huge PR. This moves most of the map navigation elements to the typescript version.
Navigation model mentioned in #4085 (comment). It supports adding new navigation items with default renderer and custom renderer. The custom renderer should probably be used only for Compass and zoom tool.

This implements a high-level API class (MapNavigationModel) that should be responsible for controlling the entire map navigation.
Each map navigation item is a MapNavigationController or has one assigned to it. MapNavigationController is responsible for holding and managing the entire state of the navigation item. Idea is to have all navigation items extend MapNavigationController and provide methods on how to control the state. It is easy to find examples of how to implement it in the converted navigation items.
Each controller has the following states defined inside them:

  • visible - visibility of the item
  • disabled - whether the item is disabled
  • pinned - item will stay on screen no matter what (unless hidden)
  • collapsed - whether the item is collapsed (item is shown in overflow menu)
  • location: "TOP" | "BOTTOM" whether the item is shown in the top or bottom area.
  • screenSize - item is visible only on mobile, desktop, or all screens
  • height and width of items, used to determine whether the item should be hidden. There is a start of implementation of measurement function but it needs to be extensively tested.
  • viewerMode - some items like Compass are visible only on the mobile screen so we set the desired ViewerMode, undefined if should be visible on all viewers

When the item has a custom renderer is set and pinned then glyph and onClick are not needed as they won't be used. If we set the custom renderer and the item can be collapsed then we should provide the glyph, onClick as they are used to render collapsed menu/panel

This provides us with:

  • we can close another tool without knowing its implementation, just get it from the model based on id and close it (measure is closed after starting pedestrian tool)
  • disable another item when one is active (starting pedestrian tool disables measure tool
  • getting information if the tool is open without getting into its implementation
  • consistent styling of the navigation items
  • collapsible navigation that goes into overflow menu on smaller screens so some items don't go out of screen.

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated CHANGES.md with what I changed.

@steve9164
Copy link
Member

Thanks for the PR. I haven't had time yet to go over this in detail but am I right in saying this PR is mostly under-the-hood changes to allow easier extension without much visual changes for a standard TerriaMap?

Deployed on our CI: http://ci.terria.io/z995-no-merge-nav/
(branch named no-merge because it builds against a custom TerriaMap branch corresponding to TerriaJS/TerriaMap#505).

@zoran995 zoran995 marked this pull request as draft January 25, 2021 10:45
Copy link
Member

@steve9164 steve9164 left a comment

Choose a reason for hiding this comment

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

I find the MyLocation.tsx -> MyLocation.ts change a bit odd, and there seems to be some things double-implemented in the MapNavigation framework (like onClick, render) and also in the "more complicated" map tools. If this makes it easier to add simple map tools I'm happy with it.

I'm interested in merging this and #5131. I'll write back tomorrow what my plan is for that.

// navigation bar has not been rendered yet so there is nothing to update.
return;
}
if (this.computeSizes.length !== this.model.enabledItems.length) {
Copy link
Member

Choose a reason for hiding this comment

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

this.computeSizes is a function so I think this condition is always true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pushed a commit with a more generic controller for navigation items, it changes the logic behind the navigation model (can drop a separate comment about changes).
IMHO most of the navigation items contained a logic that should be dropped in Models/Map and not into ReactViews. The drawback of having MyLocation as React.Component is not having the units test for it, now it is much easier to cover it with tests.

@zoran995
Copy link
Contributor Author

The last commit introduced the abstract CompositeBarModel to have a pretty much generic model for a list of rendered items that need to be controlled externally. It might be nice to connect this with the Model/Stratum system but it could also be overkill, so I went for a simple and basic API we can use.
Merging this will require a redesign of the approach in #5131 since we no longer can have HOC controlling navigation, but basically API.
For example, we can take the IElementsConfig from #5131 and on init update the required item using the model API. To hide an item we can iterate over the list during init and

terria.mapNavigationModel.hide(id);

@zoran995 zoran995 marked this pull request as ready for review March 17, 2021 12:50
@zoran995
Copy link
Contributor Author

zoran995 commented Jul 7, 2021

@steve9164 I did a basic connection between the navigation model and element visibility control. If you have a suggestion on how to improve it let me know 😃. Currently, we have only visibility control from init, but it won't be hard to implement others in the future.
Measure tool is deactivated and disabled when pedestrian tool is activated, they can't work at the same time.
Some icons are updated to support a change of color instead of using fixed color.

image
image
image

@zoran995
Copy link
Contributor Author

zoran995 commented Sep 2, 2021

@steve9164 I just went through all of this once again to confirm it is working after merging the changes, and it seems that everything is implemented as planned

@steve9164
Copy link
Member

steve9164 commented Sep 6, 2021

Thanks @zoran995 for cleaning this up. I built this locally and everything looks good, apart from one issue I found where clicks on the outer ring of the compass sometimes don't make it to the compass click handler, so dragging the outer ring to rotate the view doesn't always work.

@zoran995
Copy link
Contributor Author

zoran995 commented Sep 6, 2021

I can reproduce the same issue on main, and can't figure out what is going on. Maybe the best bet is to tsify compass in separate PR and restore jsx version here, @steve9164 what do you think?

@steve9164
Copy link
Member

steve9164 commented Sep 7, 2021

@zoran995 You're right, this exists in main. I now think the click handler always gets the clicks, but the problem is that some clicks are ignored due to the math in handleMouseDown ignoring the scale transform of the outer ring.

(Ticketed in #5799)

@steve9164 steve9164 merged commit 37a9295 into TerriaJS:main Sep 7, 2021
@na9da na9da mentioned this pull request Sep 20, 2021
9 tasks
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.

Mobx: Mobile screen button not clickable
5 participants