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

feat(geom): Add support for multi-geometries [OL5] #213

Merged
merged 1 commit into from
Apr 10, 2019

Conversation

samuel-girard
Copy link

Hello,

This change brings support for multi-geometries.
The change is to be applied only after #183 is merged.

There is just one breaking change: the coordinates input on CollectionCoordinatesComponent is not anymore limited to type [number, number][], but it supports all types of coordinates.
That means the existing code must be updated for polygons:
Old style:

<aol-feature>
    <aol-geometry-polygon>
        <aol-collection-coordinates
            [coordinates]="[[5, 45],[5.05, 45.05],[5.05, 44.95],[4.95, 44.95]]"
            [srid]="'EPSG:4326'"
        >
        </aol-collection-coordinates>
    </aol-geometry-polygon>
    <aol-style>
        <aol-style-stroke [color]="'red'"></aol-style-stroke>
        <aol-style-fill [color]="[255,0,0,0.5]"></aol-style-fill>
    </aol-style>
</aol-feature>

New style:

<aol-feature>
    <aol-geometry-polygon>
        <aol-collection-coordinates
            [coordinates]="[[[5, 45],[5.05, 45.05],[5.05, 44.95],[4.95, 44.95]]]"
            [srid]="'EPSG:4326'"
        >
        </aol-collection-coordinates>
    </aol-geometry-polygon>
    <aol-style>
        <aol-style-stroke [color]="'red'"></aol-style-stroke>
        <aol-style-fill [color]="[255,0,0,0.5]"></aol-style-fill>
    </aol-style>
</aol-feature>

Notice the [coordinates] input is now a [number, number][][], as defined in GeoJSON.

This also allows to display polygon with holes, which is not possible with the current code.

@Neonox31
Copy link
Collaborator

@samuel-girard I fixed the linting problem on next branch, could you rebase please ?

@Neonox31
Copy link
Collaborator

@samuel-girard I fixed the linting problem on next branch, could you rebase please ?

@samuel-girard samuel-girard force-pushed the feat-multigeometries branch 3 times, most recently from 17e11ca to 0e4c4c3 Compare December 17, 2018 10:24
@samuel-girard samuel-girard changed the title feat(geom): Add support for multi-geometries feat(geom): Add support for multi-geometries [OL5] Dec 17, 2018
@Neonox31 Neonox31 force-pushed the next branch 2 times, most recently from 20086b9 to ee186a8 Compare February 4, 2019 10:43
@Yakoust Yakoust force-pushed the feat-multigeometries branch from 84b7dd8 to e774111 Compare April 9, 2019 12:01
@Yakoust
Copy link
Contributor

Yakoust commented Apr 9, 2019

Hi all,

I rebase this branch on next after merging OL5 PR.
This introduce a breaking change.
It LGTM but I would like to have another review

@quentin-ol @kekel87 @dabbid

Copy link
Contributor

@kekel87 kekel87 left a comment

Choose a reason for hiding this comment

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

That seems alright to me. I made some feedback on details.

The only breaking change is to finally use the coordinates entirely and therefore correctly 😄 :

<aol-collection-coordinates [coordinates]="f.geometry.coordinates[0]"></aol-collection-coordinates>	                

become :

<aol-collection-coordinates [coordinates]="f.geometry.coordinates"></aol-collection-coordinates>

projects/ngx-openlayers/src/lib/coordinate.component.ts Outdated Show resolved Hide resolved
projects/ngx-openlayers/src/lib/coordinate.component.ts Outdated Show resolved Hide resolved
projects/ngx-openlayers/src/lib/coordinate.component.ts Outdated Show resolved Hide resolved
projects/ngx-openlayers/src/lib/coordinate.component.ts Outdated Show resolved Hide resolved
src/app/draw-polygon/draw-polygon.component.ts Outdated Show resolved Hide resolved
@Yakoust Yakoust force-pushed the feat-multigeometries branch from e774111 to d66161a Compare April 9, 2019 13:57
@Yakoust
Copy link
Contributor

Yakoust commented Apr 10, 2019

@kekel87 agree with changes?

@kekel87
Copy link
Contributor

kekel87 commented Apr 10, 2019

@Yakoust Yep i agree 👍

@Yakoust Yakoust merged commit f3a6f9c into quentinlampin:next Apr 10, 2019
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.

4 participants