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

Rename circle-pitch-scale to circle-pitch-scaling #4165

Closed
lucaswoj opened this issue Feb 1, 2017 · 8 comments
Closed

Rename circle-pitch-scale to circle-pitch-scaling #4165

lucaswoj opened this issue Feb 1, 2017 · 8 comments
Labels
breaking change ⚠️ Requires a backwards-incompatible change to the API cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.)

Comments

@lucaswoj
Copy link
Contributor

lucaswoj commented Feb 1, 2017

From @1ec5 on January 5, 2017 10:35

circle-pitch-scale should be renamed circle-scale-alignment.

mapbox/mapbox-gl-style-spec#459 (comment) proposed {circle,icon,text}-pitch-scale properties as complements to the {circle,icon,text}-{rotation,pitch}-alignment properties. As discussed in mapbox/mapbox-gl-native#6098, the circle-pitch-scale name incorrectly implies a numeric value. It’s also inconsistent with the rest of the properties:

Property a feature’s according to the map or viewport’s when the map has
circle-rotation-alignment aligns rotation rotation rotation
circle-pitch-alignment aligns pitch pitch pitch
circle-pitch-scale aligns scale scale pitch

Renaming circle-pitch-scale to circle-scale-alignment would make the property’s type more obvious and better harmonize the name with the others:

Property does what a feature’s according to the map or viewport’s when
circle-scale-alignment aligns scale scale pitch

/cc @jfirebaugh @lucaswoj

Copied from original issue: mapbox/mapbox-gl-style-spec#645

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Feb 1, 2017

From @1ec5 on January 5, 2017 10:37

An alternative to *-scale-alignment would be *-size-alignment, which would be consistent with icon-size and text-size.

@lucaswoj lucaswoj added the breaking change ⚠️ Requires a backwards-incompatible change to the API label Feb 1, 2017
@1ec5 1ec5 added the cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) label Feb 3, 2017
@jfirebaugh
Copy link
Contributor

I don't agree with this proposal.

  • circle-rotation-alignment -- there is no such property. Circles have radial symmetry and no rotation properties.
  • circle-scale-alignment doesn't contain the word "pitch". The property specifies the scaling behavior of circles when the map pitches; it should have both "pitch" and "scale" in its name.

@1ec5
Copy link
Contributor

1ec5 commented Mar 10, 2017

circle-rotation-alignment -- there is no such property.

Sorry, I picked a poor example there. Here’s a revised table:

Property a feature’s according to the map or viewport’s when the map is
icon-rotation-alignment aligns rotation rotation rotated
icon-pitch-alignment aligns pitch pitch tilted
icon-pitch-scale aligns scale scale tilted (with pitch)

I had picked circle-* because we haven’t implemented icon-pitch-scale or text-pitch-scale yet (#4120 (comment)).

The property specifies the scaling behavior of circles when the map pitches; it should have both "pitch" and "scale" in its name.

Perhaps circle-size-pitch-alignment would address that discoverability concern? (It also parallels icon-size, in a way.)

It just feels weird to me that the existing property implies a numeric “scale” value but is set to an enum, whereas related enum-typed properties are “alignments” instead of “scales”.

@jfirebaugh
Copy link
Contributor

I don't think of the scaling behavior as an "alignment", so including that word in the property seems odd. How about circle-pitch-scaling?

@1ec5
Copy link
Contributor

1ec5 commented Mar 10, 2017

That could work. (Though I’d still hold out some hope for circle-pitch-sizing. We have other properties with the word “size”, none with “scale”, and there’s box-sizing in CSS.)

@1ec5 1ec5 changed the title Rename circle-pitch-scale to circle-scale-alignment Rename circle-pitch-scale to circle-pitch-scaling Mar 10, 2017
@ChrisLoer
Copy link
Contributor

One other wrinkle from working on text-pitch-scaling is that we're considering making the property a continuous variable over [0,1] (so that a style designer can choose to make items in the distance "somewhat smaller, but still legible"). IMO, that pushes even more against alignment as a descriptor.

I like text-pitch-scaling because it implies (1) that "scaling" is done to a base size that's set separately, (2) that scaling is done in response to pitch.

/cc @nickidlugash

@ChrisLoer
Copy link
Contributor

One other wrinkle from working on text-pitch-scaling is that we're considering making the property a continuous variable over [0,1] (so that a style designer can choose to make items in the distance "somewhat smaller, but still legible").

In the latest version of that PR, we are hardwiring the value, thus avoiding the naming problem entirely (at least for text/icons)

@jfirebaugh
Copy link
Contributor

Closing here; see #6584.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ Requires a backwards-incompatible change to the API cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.)
Projects
None yet
Development

No branches or pull requests

4 participants