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

Set select style (vertex, halo) in feature helper, used in gmf-drawfeature #1016

Merged
merged 1 commit into from Apr 15, 2016

Conversation

adube
Copy link
Contributor

@adube adube commented Apr 11, 2016

This PR adds the support of drawing vertex in addition to the original style in the feature helper. The gmf-featurestyle directive uses it, in addition to the gmf-drawfeature one.

Geometries that have the vertex drawn:

  • LineString
  • Polygon
  • Rectangle

Geometries that don't have vertex drawn:

  • Circle
  • Point
  • Text

An additional "halo" style is added to selected features.

As a bonus, this PR also includes the ngeo.interaction.ModifyCircle interaction. Circles are now properly modified.

Todo

  • add white border around features
  • use square for modify style
  • add modify circle interaction
  • rebase onto master once GMF draw feature directive #1001 is merged
  • code review
  • apply reviewer's required corrections
  • travis pass

Live example

@adube
Copy link
Contributor Author

adube commented Apr 12, 2016

@fgravin Ready for review.

@sbrunner
Copy link
Member

Shouldn't be drown on Rectangle... or is it planed to use them to resize the rectangle?

Disabling moving vertex Rectangle and Circle on will come in an other pull request?

@adube
Copy link
Contributor Author

adube commented Apr 12, 2016

  • resize rectangle: Yes, it will be possible
  • moving vertex of Rectangle and Circle: Yes, it will be covered in an other PR

@sbrunner
Copy link
Member

OK, tranks :-)

@fgravin
Copy link
Member

fgravin commented Apr 12, 2016

Regarding @sbrunner comment, it is true that the geometry editing style is a blue point, maybe it should be the same style as the vertixes ?

@adube
Copy link
Contributor Author

adube commented Apr 12, 2016

@pgiraud, see previous comment.

@adube
Copy link
Contributor Author

adube commented Apr 13, 2016

@fgravin - ping (ready for review)
@pgiraud - ping (regarding sbrunner's comment)

@fgravin
Copy link
Member

fgravin commented Apr 13, 2016

From my side, the code is ok.
For the look and feel, could you try to add a third style when selected, with a stroke width +=2, to simulate an halo ?

For the other things, see with @pgiraud

@adube
Copy link
Contributor Author

adube commented Apr 13, 2016

Halo --> Yep.

@pgiraud
Copy link
Contributor

pgiraud commented Apr 13, 2016

I finally agree with @fgravin that the blue point is a bit awkward. Can you please make it so it uses the same style as the vertices (black square)?

@adube
Copy link
Contributor Author

adube commented Apr 13, 2016

Black square --> Yep.

@adube adube force-pushed the gmf-drawfeature-vertex branch 4 times, most recently from 74195df to 58cd0e8 Compare April 13, 2016 19:45
@adube
Copy link
Contributor Author

adube commented Apr 13, 2016

Ready for review.

@pgiraud, because fgravin is not here until next week would you please complete the code review, or find someone who's willing to do it ? Thanks.

See the live demo.

@adube adube changed the title Support vertex in feature helper Set select style (vertex, halo) in feature helper, used in gmf-drawfeature Apr 13, 2016
@pgiraud
Copy link
Contributor

pgiraud commented Apr 15, 2016

The UI/UX looks very good. Hooray!
A nice-to-have future enhancement would be to show the halo when hovering the features.

Giving a look at the code now.

color: [0, 0, 0, 1]
})
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

The style used here is the same as the style used for vertices. This should be factorized in my opinion.

@pgiraud
Copy link
Contributor

pgiraud commented Apr 15, 2016

I added minor comments. Otherwise it looks very good.

@adube
Copy link
Contributor Author

adube commented Apr 15, 2016

Thanks for the review. I'll apply the required changes and report back.

@adube
Copy link
Contributor Author

adube commented Apr 15, 2016

All of the corrections required by the reviewer have been made. Ready for merge.

@pgiraud
Copy link
Contributor

pgiraud commented Apr 15, 2016

Looks good to me. Merging.

@pgiraud pgiraud merged commit ae59d24 into camptocamp:master Apr 15, 2016
@adube adube deleted the gmf-drawfeature-vertex branch April 15, 2016 14:42
@sbrunner sbrunner added this to the Older milestone Aug 23, 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.

None yet

4 participants