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

Proposal: add icon-anchor property #5183

Merged
merged 2 commits into from
Aug 25, 2017
Merged

Proposal: add icon-anchor property #5183

merged 2 commits into from
Aug 25, 2017

Conversation

lbud
Copy link
Contributor

@lbud lbud commented Aug 23, 2017

This PR adds icon-anchor, which behaves exactly like text-anchor. There is no existing ticket on adding icon-anchor 🙊 but I think it's useful in doing things like aligning SDF pins/markers to point to their source Point rather than always being centered/requiring extra space. Incidentally, this was also discussed recently in #5129 (comment). The implementation is very straightforward as it just reuses text-anchor code.

If everyone is 👍 on this I'll work on a native PR as well.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page
  • mapbox-gl-native PR Add icon-anchor property mapbox-gl-native#9849
  • update SDK support tables

Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

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

Looks great!

When I was trying to make sure I understood how the changes worked, I modified some of the tests to use text-anchor and icon-anchor simultaneously with different values. As far as I can tell, the two work totally independently without any problem. Is there any case where we'd want something like an "auto" value for icon-anchor that followed the value of text-anchor? I'm thinking of a situation where you're using icon-text-fit to use an icon as background for a label.

Just curious, why is the branch called iconOffset? You were starting to work on #5129?

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

👍 Kind of surprising we didn't implement this until now.

@lbud
Copy link
Contributor Author

lbud commented Aug 23, 2017

Just curious, why is the branch called iconOffset

🤦‍♀️ just a typo 😬 @ChrisLoer — this was totally unrelated to #5129, have not touched that.

Is there any case where we'd want something like an "auto" value for icon-anchor that followed the value of text-anchor? I'm thinking of a situation where you're using icon-text-fit to use an icon as background for a label.

Good question — @nickidlugash? I'd be inclined to say no as it seems a bit prescriptive about combining text and icons — when using an icon as a background for a label you'd want both to use the same anchor, but for e.g. placing a label underneath an icon (in the y direction rather than z) you could use icon-anchor: bottom and text-anchor: top (this could potentially be an improvement on the way I assume designers currently do it, using -offset properties).

@nickidlugash
Copy link

Awesome! 🎉

Is there any case where we'd want something like an "auto" value for icon-anchor that followed the value of text-anchor? I'm thinking of a situation where you're using icon-text-fit to use an icon as background for a label.

Good question — @nickidlugash? I'd be inclined to say no as it seems a bit prescriptive about combining text and icons

I think I would agree with @lbud here –  icon+text position relationships are varied enough across use cases that I think it's better to stay agnostic. I think an auto value is most useful in the spec when there is a clear case for it to be the default value, which I don't think there is here.

@lbud lbud merged commit 5f4d86d into master Aug 25, 2017
@lbud lbud deleted the iconOffset branch August 25, 2017 17:41
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