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

Give precedence to feature styles #2394

Merged
merged 5 commits into from Jul 25, 2014
Merged

Conversation

tonio
Copy link
Member

@tonio tonio commented Jul 16, 2014

Fixes #1999.

It also:

  • move default styles to an ol.style.defaults namespace
  • move StyleFunction definition & createStyleFunction from ol.feature to ol.style

@bartvde bartvde added this to the v3.0.0-gamma.3 milestone Jul 16, 2014
layer.setStyle(style);
expect(layer.getStyleFunction()).to.be.a('function');
expect(layer.getStyleFunction()).not.to.be(
ol.feature.defaultStyleFunction);
Copy link
Member

Choose a reason for hiding this comment

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

There is no ol.feature.defaultStyleFunction any more, right? I think this should be ol.style.defaults.styleFunction.

Copy link
Member

Choose a reason for hiding this comment

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

You can cherry-pick c1d49a8 to fix this.

@ahocevar
Copy link
Member

I added a test to make sure that feature styles take precedence. Please cherry-pick 16cdd35 if you agree.

@ahocevar
Copy link
Member

This looks good @tonio. Two cherry-picks in the comments above, and you may want to change the name of src/ol/style/defaultsstyle.js to src/ol/style/styledefaults.js. Please merge when addressed.

@tonio
Copy link
Member Author

tonio commented Jul 24, 2014

Thanks for the help on this Andreas. Wasn’t clear to me on how to write test for this.
I’m unsure about the file renaming. Even if the double s doesn’t looks good, it wouldn’t be coherent with other files to change it, no fillstyle.js, iconstyle.js

@ahocevar
Copy link
Member

When you speak about styles, you e.g. give your polygon a "fill style". And if you don't, your features will be rendered with the "style defaults". Or the "default style". The former would suggest renaming the file, the latter would suggest renaming ol.style.defaults.styleFunction to ol.style.defaultStyleFunction.

@probins
Copy link
Contributor

probins commented Jul 24, 2014

what about the default editing styles? Shouldn't they be moved too?

@bartvde
Copy link
Member

bartvde commented Jul 25, 2014

@tonio are you planning to have this in for the gamma.3 release?

@tonio
Copy link
Member Author

tonio commented Jul 25, 2014

I’ll try to make the asked changes this morning, early enough for you?

On 25 July 2014 09:24, Bart van den Eijnden notifications@github.com
wrote:

@tonio https://github.com/tonio are you planning to have this in for
the gamma.3 release?


Reply to this email directly or view it on GitHub
#2394 (comment).

Antoine Abt

Camptocamp France SAS
Savoie Technolac, BP 352
73377 Le Bourget du Lac, Cedex

Tel : 00 33 4 79 44 44 94
Mail : antoine.abt@camptocamp.com
http://www.camptocamp.com

@bartvde
Copy link
Member

bartvde commented Jul 25, 2014

sure that is fine, I'll do the release in the afternoon

@tonio
Copy link
Member Author

tonio commented Jul 25, 2014

Defaults styles now moved to ol.style.defaultStyleFunction.

@bartvde
Copy link
Member

bartvde commented Jul 25, 2014

@tonio is there more to be done for this PR to be merged?

@tonio
Copy link
Member Author

tonio commented Jul 25, 2014

@bartvde not in my opinion.

On 25 July 2014 10:42, Bart van den Eijnden notifications@github.com
wrote:

@tonio https://github.com/tonio is there more to be done for this PR to
be merged?


Reply to this email directly or view it on GitHub
#2394 (comment).

Antoine Abt

Camptocamp France SAS
Savoie Technolac, BP 352
73377 Le Bourget du Lac, Cedex

Tel : 00 33 4 79 44 44 94
Mail : antoine.abt@camptocamp.com
http://www.camptocamp.com

@ahocevar
Copy link
Member

I think this looks good now. Thanks @tonio!

@probins
Copy link
Contributor

probins commented Jul 25, 2014

with this change you have default style defined in style, and default editing style defined in feature; this seems inconsistent to me

@bartvde
Copy link
Member

bartvde commented Jul 25, 2014

@probins can you create a separate ticket for discussing this and set the milestone to gamma.4? TIA.

I am going to merge this now since I am about to do the gamma.3 release.

bartvde added a commit that referenced this pull request Jul 25, 2014
Give precedence to feature styles
@bartvde bartvde merged commit f468239 into openlayers:master Jul 25, 2014
@probins
Copy link
Contributor

probins commented Jul 25, 2014

@bartvde I can't set the milestone; I'll leave that to you

@bartvde
Copy link
Member

bartvde commented Jul 25, 2014

thanks @probins I have set the milestone

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.

Give precedence to feature styles
4 participants