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

#2215: add group in TOC toolbar #3797

Merged
merged 3 commits into from
May 29, 2019
Merged

Conversation

mbarto
Copy link
Contributor

@mbarto mbarto commented May 24, 2019

Description

Improvements to the TOC:

  • NEW add group button (when status is DESELECTED or a single group is selected)
  • add layer button (when status is DESELECTED or a single group is selected) --> adds a layer directly to the selected group

Issues

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
Adding a new group or adding a layer to an existing group is complicated.

What is the new behavior?
Adding a new group or adding a layer to an existing group is easier.

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@coveralls
Copy link

coveralls commented May 24, 2019

Coverage Status

Coverage decreased (-27.01%) to 54.398% when pulling 71b2235 on mbarto:add_groups into 410fb7d on geosolutions-it:master.

@tdipisa tdipisa requested review from MV88 and offtherailz and removed request for MV88 May 24, 2019 11:55
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Here the things I noticed:

  • When I delete a layer, all new groups are removed

  • If I use a . (dot) as the name of the group (for instance geo-solutions.it), then I open the layer properties, the groups selector i see geo-solutions/it
    image

  • If a group is selected we are going to add the layer/group from the button as a sub-group. Maybe in this case the tooltip should show something like "add sub-group to the selected group" or "add layer to the selected group". Using the name of the group in the tooltip is a plus.

  • Wrong tooltip for group properties
    image

  • Wrong tooltip for group visibility button (I verified this issue already exists on dev)
    image

Here some things that may be confusing:

  • When I open the catalog from the TOC the layer is added to the selected group, when I do it from the menu, the layer is added to the "Default" group. This may confuse the user.
  • When I select a group to add layers to it, all it's layers in the group are selected, too. When I add a new layer to the selected group, this new layer is not selected. This looks somehow correct to me, but may be confusing. Just wanted to make you notice it.

This gif describes both layer selction and add from catalog issues:
selection

* @param {object} properties the properties to set, as key (property name), value pairs
* @return {object} of type `SET_CONTROL_PROPERTIES` with control and properties
*/
function setControlProperties(control, ...properties) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not directly the object? I think is more explicit that the array:

setControlProperties('mycontrol', 'a', 1, 'b', 2); // less intuitive
setControlProperties('mycontrol', {a:1, b: 2}) // more intuitive

Notice. The documentation tells something different. I didn't try yet, but documenting spread operator may work using this syntax @param {...object} myParam (JSDoc 3.4.3). Please try (or refactor as suggested)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because using ...properties is more functional friendly (partial application) and in fact my first usage of it took advantage of partial application. Eventually we can also support both syntaxes

Copy link
Member

@offtherailz offtherailz May 28, 2019

Choose a reason for hiding this comment

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

Now I see. That's cool 👍
I should not provide both to avoid confusion.

So you could one of the following:

  1. Change the documentation to specify better how to use the action, maybe with an example
  2. Make the properties an object and change all the .bind with something like
( properties ) => setControlProperties('myControl', {  {a: 1, b: 2}, ...properties} }

Do what you think is better.

@mbarto
Copy link
Contributor Author

mbarto commented May 27, 2019

@offtherailz "When I open the catalog from the TOC the layer is added to the selected group, when I do it from the menu, the layer is added to the "Default" group. This may confuse the user."
What you suggest instead?

…ayer removed, fixed problem with dot in group name
@offtherailz
Copy link
Member

@mbarto I'd like to see the opinion of @tdipisa about it. Anyway I can suggest to set the tooltip "Add layer to the selected group", that clarifies a little bit. I think that it could be enough.

Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Good. Almost done
Please look at my comments (1 inline, 1 on PR) and try to solve translation file conflict

@mbarto mbarto merged commit 50c18da into geosolutions-it:master May 29, 2019
@mbarto mbarto deleted the add_groups branch May 29, 2019 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow creation of new groups / subgroups from the TOC
4 participants