Skip to content

Fix #3224 Style Editor Plugin (GeoCSS)#3273

Merged
allyoucanmap merged 12 commits intogeosolutions-it:c127_geonode_integrationfrom
allyoucanmap:c127_geonode_integration
Nov 6, 2018
Merged

Fix #3224 Style Editor Plugin (GeoCSS)#3273
allyoucanmap merged 12 commits intogeosolutions-it:c127_geonode_integrationfrom
allyoucanmap:c127_geonode_integration

Conversation

@allyoucanmap
Copy link
Copy Markdown
Contributor

@allyoucanmap allyoucanmap commented Oct 23, 2018 •
edited
Loading

Description

This PR will introduce Style Editor in GeoCSS and improve some components as TOCItemsSettings.

TOCItemsSettings improvements:

  • externalized update params to global state
  • added onClose and onClick function to the tab component
  • added locked flag to disable inactive tabs and hide close button

In addition a section of the state has been dedicated to additional layers.
Additional layers will override propertities of layers without modify the original layer object.
(see layerSelectorWithMarkers function in /selectors/layer.js)

Status docs and test:

  • Added additional layers actions, selectors and reducers
  • Improved tabs management of TOCItemsSettings and updated settings params function
  • Style Editor plugin

Issues

Please check if the PR fulfills these requirements

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

  • Feature

What is the current behavior? (You can also link to an open issue here)
Issue #3224

What is the new behavior?

  • Added functionality to support Style Editor

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

  • Yes
  • No

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

Other information:

@ghost ghost assigned allyoucanmap Oct 23, 2018
@coveralls
Copy link
Copy Markdown

coveralls commented Oct 23, 2018 •
edited
Loading

Coverage Status

Coverage decreased (-0.3%) to 80.897% when pulling 37add42 on allyoucanmap:c127_geonode_integration into f09785f on geosolutions-it:c127_geonode_integration.

@geosolutions-it geosolutions-it deleted a comment Oct 24, 2018
@geosolutions-it geosolutions-it deleted a comment Oct 26, 2018
@allyoucanmap allyoucanmap changed the title WIP Fix #3224 Style Editor Plugin (GeoCSS) Fix #3224 Style Editor Plugin (GeoCSS) Oct 30, 2018
@allyoucanmap allyoucanmap requested a review from mbarto October 30, 2018 08:51
@geosolutions-it geosolutions-it deleted a comment Oct 31, 2018
@mbarto
Copy link
Copy Markdown
Contributor

mbarto commented Oct 31, 2018

@allyoucanmap I see that we have the old WMSStyle functionalities replaced by the StyleEditor when StyleEditor is configured.
I think the read-only version of StyleEditor could replace WMSStyle completely, is that possible, so that we can remove WMSStyle?

@mbarto
Copy link
Copy Markdown
Contributor

mbarto commented Oct 31, 2018

problem: when the TOCItemSettings is "reset", but is locked (for example try to change the language while you are editing a style) the TOC is still locked, so you cannot do anything

@mbarto
Copy link
Copy Markdown
Contributor

mbarto commented Oct 31, 2018

A question: I see that you cannot edit the default style, but you can edit available styles that are not the default, also if those are not created using the styler. Is this by design?

@mbarto
Copy link
Copy Markdown
Contributor

mbarto commented Oct 31, 2018

When you create a new style as "Base SLD" you get an error from geoserver if you don't have the CSS module, because content-type is set to application/vnd.geoserver.geocss+css.
I expect that Base SLD creates an SLD style.

@mbarto
Copy link
Copy Markdown
Contributor

mbarto commented Oct 31, 2018

Errors are not catched / shown properly when creating a new style, when GS security is not set properly you cannot create a style, but the user is not notified of the problem.

mbarto
mbarto suggested changes Oct 31, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@mbarto mbarto left a comment

Choose a reason for hiding this comment

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

I am not able to do a real code review, such a big work cannot be just "read" and commented.
I downloaded the branch and did some hands-on testing, finding some issues, and I added comments for those.
I think big tasks like this one should be implemented by more than one person, so that a real code review can happen in real time, or we should commit them in small batches that a human can understand in a reasonable amount of time,
That said, I could not spot any particular error / issue during my reading, apart from the already mentioned comments.

@geosolutions-it geosolutions-it deleted a comment Nov 2, 2018
@allyoucanmap allyoucanmap merged commit 16a3542 into geosolutions-it:c127_geonode_integration Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mbarto mbarto mbarto approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@allyoucanmap allyoucanmap

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@allyoucanmap @coveralls @mbarto