[#5008] More granular control over tools/panels #5131

smotornyuk · 2021-01-11T17:50:25Z

Add ability to hide different buttons\panels using init files

This PR adds a new wrapper visibilitySwitch that conditionally renders elements depending on their configuration from init-file. Inside init-file, user can add new elements sections that looks like the following:

  "elements": {
    "show-workbench": {"visible": false},
    "map-data-count": {"visible": false},
    "help": {"visible": false},
    "menu-bar": {"visible": false},
    "map-navigation": {"visible": true},
    "compass": {"visible": false},
    "zoom": {"visible": true},
    "my-location": {"visible": false},
    "split-tool": {"visible": false},
    "about-link": {"visible": false},
    "measure-tool": {"visible": false},
    "feedback": {"visible": false},
    "related-maps": {"visible": false},
    ...
  },

Later these config objects are passed by-name into corresponding elements(buttons, panels), and depending on the visible flag they either rendered or not. This is a rather hot-fix and I want to provide a more elegant solution, but it requires more essential changes. Here a few points that are not completely clear to me:

Configuration for elements stored inside Terria instance. How to make it reactive, so that it updates every time user changes init file(for example, retypes #<INIT> part of the URL). Right now only initial values, that were available at load time are applied
From my point of view, it has the sense to use the same common React component for all of the buttons\controls and pass different props\children in order to style those buttons. Right now only a few elements are using MapIconButton, all others are using a unique way of rendering.
Maybe we can pass terria/viewState config object through context so that it will be possible to access it using hooks inside any component?
Where is the best place to put visibilitySwitch from the conceptual point of view? Maybe, HOCs?
Is there any sense in collecting all the names of elements with controlled visibility into some kind of service? For example, developers can use this service for listing all the available names and(potentially) for switching states of every separate component in real-time.

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.

CLAassistant · 2021-01-11T17:50:30Z

All committers have signed the CLA.

zoran995 · 2021-01-11T19:09:21Z

Hi @smotornyuk
There is an option to show/hide buttons from config options; are you looking for this as another option to disable buttons?

I didn't have much time to look into this in detail, but I like the idea behind this. I am wondering how will this play with the option to specify another version of catalog init file (extend it or completely replace it) in the URL which one will take effect (if the administrator doesn't want to have some button active, end-user shouldn't have an option to override that setting - unless there is also catalog option which can't be changed).

Some other things that should be taken into account, mainly working on making the navigation more accessible and easier to integrate new buttons, especially #5020, #5062, and discussion in #4085. I expect it to be much easier to implement more general visibility control with that, at least for the navigation bar. Also, there is an initiative of making terriajs work as a multitenant solution, but I don't know how it is going to work.

Let's see what TerriaJS team says about this.

steve9164 · 2021-02-10T06:18:43Z

Hey @smotornyuk. I really like the idea of this PR. I'll do a full code review of this hopefully tomorrow.

In response to your questions:

Element config reactivity: I'll have to look further into the code to answer this
Inconsistency in drawing of buttons: I don't know of any reasons not to use MapIconButtons for everything. It's likely that buttons that have been modified recently use the new MapIconButton and others don't and should be updated.
Passing terria/viewState through React context and access with hooks: We should do this. It will slightly change how e.g. tests are written (components in jsx/tsx will have to be wrapped in the context provider when not rendered inside the Terria UI) but I think it'll make developing the UI easier.
Where should visibilitySwitch go? HOCs is the right place.
Where should we list all of the elements that can be configured? At the moment I think we should add a page to our docs https://docs-v8.terria.io about the functionality and list the element names there. (PRs to mobx-docs branch for updated docs would be appreciated. I'll try to get that merged onto next soon too).

steve9164

Hi @smotornyuk. Thanks for the PR. I think Terria.elements is only not reactive due to limits of MobX v4 (see the dot point about MobX 4 in MobX object docs if you're interested). It should be reactive if you switch to using an ObservableMap.
Other than that I've added some suggestions for small improvements to change/discuss.

steve9164 · 2021-02-12T08:41:00Z

lib/Models/Terria.ts

@@ -175,6 +176,9 @@ export default class Terria {
  readonly timelineClock = new Clock({ shouldAnimate: false });
  // readonly overrides: any = overrides; // TODO: add options.functionOverrides like in master

+  @observable
+  readonly elements = new ElementsConfig();


I think instead of an object this should be an ObservableMap:

Suggested change

readonly elements = new ElementsConfig();

readonly elements = new observable.map<String, IElementConfig>();

Yep, this way it works as expected

steve9164 · 2021-02-12T08:41:03Z

lib/Models/Terria.ts

@@ -822,6 +826,10 @@ export default class Terria {
      this.catalog.group.addMembersFromJson(stratumId, initData.catalog);
    }

+    if (isJsonObject(initData.elements)) {
+      Object.assign(this.elements, initData.elements);


If you agree with suggestion to use an ObservableMap, you can use the MobX function ObservableMap.merge:

Suggested change

Object.assign(this.elements, initData.elements);

this.elements.merge(initData.elements);

steve9164 · 2021-02-12T08:45:29Z

lib/ReactViews/Map/MapNavigation.jsx

-                  <ZoomControl terria={this.props.terria} />
+                  <ZoomControl
+                    terria={this.props.terria}
+                    elementConfig={this.props.terria.elements["zoom"]}


These will become this.props.terria.elements.get("zoom") if you use a Map.

steve9164 · 2021-02-12T08:52:41Z

lib/ReactViews/StandardUserInterface/customizable/VisibilitySwitch.jsx

+  function VisibilitySwitch(props) {
+    const isVisible = props.elementConfig ? props.elementConfig.visible : true;
+    return isVisible ? <Component {...props} /> : null;


Could be written as:

Suggested change

function VisibilitySwitch(props) {

const isVisible = props.elementConfig ? props.elementConfig.visible : true;

return isVisible ? <Component {...props} /> : null;

function VisibilitySwitch({ elementConfig, ...props }) {

const isVisible = elementConfig ? elementConfig.visible : true;

return isVisible ? <Component {...props} /> : null;

to avoid passing elementConfig as a prop to inner component

steve9164 · 2021-02-12T08:55:54Z

lib/ReactViews/StandardUserInterface/customizable/VisibilitySwitch.jsx

@@ -0,0 +1,22 @@
+import React from "react";


This file should be moved to HOCs

zoran995 · 2021-02-14T11:45:16Z

Hey, @steve9164 we also like the idea of introducing more controls of navigation items but I am still more into introducing the navigation model component that would control all navigation items. It would probably reduce the complexity of the component rendering, and also force us to have all of the controls for the new navigation item.

The one written in #5062 is just a quickly written prototype for a client on how it could look and work (the basic idea is taken from vs code models), and I am aware that some parts of it should go through a rewrite and extended on menu bar. The nav model also might be connected to Stratum and Traits system so we benefit from multiple layers of configurability. We should probably separate it into two parts one just to introduce the model, and then grouping of items on smaller screens. If you agree with proposed and @smotornyuk is interested and have time I will be happy to let him take it over.

steve9164 · 2021-02-15T14:17:30Z

Hey @zoran995. I think you're right. It would be beneficial to have the navigation controls framework reviewed before merging this. I'll give it a review this week.

smotornyuk · 2021-03-08T08:03:32Z

@steve9164, thank you for review. observable.map really fixed the thing:) And I've applied other changes

I'm not sure about this place - i had to extend type with any because otherwise, I'm constantly getting the following error and solutions from StackOverflow don't look nice either

ERROR in packages/terriajs/lib/ReactViews/HOCs/withControlledVisibility.tsx:19:57
TS2700: Rest types may only be created from object types.
    17 | ) => {
    18 |   // eslint-disable-next-line require-jsdoc
  > 19 |   function WithControlledVisibility({ elementConfig, ...props }: P & WithControlledVisibilityProps) {
       |                                                         ^^^^^
    20 |     const isVisible = elementConfig ? elementConfig.visible : true;
    21 |     return isVisible ? <WrappedComponent {...props} /> : null;
    22 |   }

AnaBelgun · 2021-03-31T03:10:46Z

@steve9164 this will need to be checked and merged by 12-13 April max; the dashboard depending on it will be launched mid April and it needs a couple of days to test the data etc etc

smotornyuk added 2 commits January 11, 2021 19:22

Add missing files

11c2ff7

Add visibilitySwitch wrapper

554d336

AnaBelgun added the P-PacificMap label Jan 13, 2021

steve9164 self-assigned this Feb 1, 2021

AnaBelgun added the High priority label Feb 5, 2021

steve9164 requested changes Feb 12, 2021

View reviewed changes

changes after review

ae96217

zoran995 mentioned this pull request Mar 17, 2021

New version of Map Navigation #5062

Merged

2 tasks

smotornyuk marked this pull request as ready for review April 1, 2021 04:27

Rename IElementsConfig to singular form

da47014

steve9164 merged commit dec1cd4 into TerriaJS:next Apr 13, 2021

iaroslav13 mentioned this pull request Nov 9, 2021

PacificMap - Sep 2021 release PacificCommunity/Terria-PacificMap#71

Closed

6 tasks

steve9164 mentioned this pull request May 6, 2022

More granular control over tools/panels #5008

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5008] More granular control over tools/panels #5131

[#5008] More granular control over tools/panels #5131

smotornyuk commented Jan 11, 2021

CLAassistant commented Jan 11, 2021 •

edited

zoran995 commented Jan 11, 2021

steve9164 commented Feb 10, 2021

steve9164 left a comment

steve9164 Feb 12, 2021 •

edited

smotornyuk Mar 8, 2021

steve9164 Feb 12, 2021

steve9164 Feb 12, 2021

steve9164 Feb 12, 2021

steve9164 Feb 12, 2021

zoran995 commented Feb 14, 2021

steve9164 commented Feb 15, 2021

smotornyuk commented Mar 8, 2021

AnaBelgun commented Mar 31, 2021

	readonly elements = new ElementsConfig();
	readonly elements = new observable.map<String, IElementConfig>();

	Object.assign(this.elements, initData.elements);
	this.elements.merge(initData.elements);

[#5008] More granular control over tools/panels #5131

[#5008] More granular control over tools/panels #5131

Conversation

smotornyuk commented Jan 11, 2021

Add ability to hide different buttons\panels using init files

Checklist

CLAassistant commented Jan 11, 2021 • edited

zoran995 commented Jan 11, 2021

steve9164 commented Feb 10, 2021

steve9164 left a comment

Choose a reason for hiding this comment

steve9164 Feb 12, 2021 • edited

Choose a reason for hiding this comment

smotornyuk Mar 8, 2021

Choose a reason for hiding this comment

steve9164 Feb 12, 2021

Choose a reason for hiding this comment

steve9164 Feb 12, 2021

Choose a reason for hiding this comment

steve9164 Feb 12, 2021

Choose a reason for hiding this comment

steve9164 Feb 12, 2021

Choose a reason for hiding this comment

zoran995 commented Feb 14, 2021

steve9164 commented Feb 15, 2021

smotornyuk commented Mar 8, 2021

AnaBelgun commented Mar 31, 2021

CLAassistant commented Jan 11, 2021 •

edited

steve9164 Feb 12, 2021 •

edited