Skip to content

Fix #3696 Various fixes, utils and drawSupport#3697

Merged
MV88 merged 2 commits intogeosolutions-it:masterfrom
MV88:3696_various_fixes
Apr 29, 2019
Merged

Fix #3696 Various fixes, utils and drawSupport#3697
MV88 merged 2 commits intogeosolutions-it:masterfrom
MV88:3696_various_fixes

Conversation

@MV88
Copy link
Copy Markdown
Contributor

@MV88 MV88 commented Apr 17, 2019

Description

this pr provides fixes in CoordinateUtils, MapUtils, and it removes various warnings from console

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)
see issue

What is the new behavior?
you can draw marker from "start" drawStatus, and "Marker" DrawMethod
getGeoJSONExtent works with simple Feature

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:
can cause conflicts with Annotations branch

@MV88 MV88 requested a review from mbarto April 17, 2019 13:35
@MV88 MV88 self-assigned this Apr 17, 2019
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 17, 2019

Coverage Status

Coverage increased (+0.2%) to 81.222% when pulling 0e8973f on MV88:3696_various_fixes into cfd7417 on geosolutions-it:master.

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.

Looks good, just one minor question

return [...this.props.layers, ...this.props.additionalLayers].filter(this.filterLayer).map((layer, index) => {
return (
<plugins.Layer type={layer.type} srs={projection} position={index} key={layer.id || layer.name} options={layer} securityToken={this.props.securityToken}>
<plugins.Layer type={layer.type} srs={projection} position={index} key={layer.id || layer.name || uuidv1()} options={layer} securityToken={this.props.securityToken}>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What the use case for a layer without both id and name? Do you have an example?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

when you use a vector layer in the additionalLayers list

Copy link
Copy Markdown
Member

@offtherailz offtherailz Apr 18, 2019

Choose a reason for hiding this comment

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

I suggest to add unique id to every additional layer you create. I suggest to assign one on creation. If missing, please use a default one in reducer, @mbarto do you agree?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@offtherailz i'm just passing a random value if it does not exist to the key prop to avoid have a warning in the console

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The warning, in that case, is correct. If some layer has id or name missing, probably you're doing something wrong, and it is better to be notified of this.

@geosolutions-it geosolutions-it deleted a comment Apr 23, 2019
@tdipisa tdipisa added this to the 2019.02.00 milestone Apr 29, 2019
@MV88 MV88 merged commit 1772771 into geosolutions-it:master Apr 29, 2019
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.

Various problems and improvements (warnings in console, draw marker, getGeoJSONExtent bug)

5 participants