Annotation labels #719

manthey · 2017-07-17T19:28:11Z

Support annotation labels.

This adds a new feature type, geo.textFeature, where the data items have a position, text, and styling. Currently this is only available for the canvas renderer. Annotations now have label, showLabel, labelStyle, and description properties. The annotation layer has a showLabels option for toggling all labels. This supports rotation, scaling, offset, shadows, and strokes on text features.

The annotation example exposes all of these options.

Documentation and tests are fully updated.

This adds a new feature type, `geo.textFeature`, where the data items have a position, text, and styling. Currently, this is only available for the canvas renderer, and not all styles are hooked through. Annotations now have `label`, `showLabel`, `labelStyle`, and `description` properties. The annotation layer has a `showLabels` option for toggling all labels. Update documentation of annotation.js and annotationLayer.js. This needs to process and pass through styles to the renderer. It needs to generate a synthetic font style if the font substyles are used. It needs to support rotation, scaling, and offset on text features. It needs tests. The annotation example should allow label adjustments, including determining where placement is based from. Placement basis needs to be an annotation option.

This handles font substyles, such as fontSize. This is exposed in the annotations example. This also handles textAlign, textBaseline, color, and textOpacity. Much of the infrastructure for other text styles is in place.

This adds a utility function to combine a color with an optional opacity and an opacity value into a single color with opacity value.

Better handle blanks in the annotations example. Show a show label option in the annotations example.

Remove the direction property -- it isn't supported by Firefox and only obscurely supported by Chrome.

aashish24

Looking very good @manthey now I am just going through the code and details.

aashish24 · 2017-07-20T21:51:34Z

src/textFeature.js

@@ -0,0 +1,207 @@
+var inherit = require('./inherit');


the API is nice @manthey

aashish24 · 2017-07-20T22:03:29Z

src/canvas/textFeature.js

+   * @private
+   */
+  var m_this = this,
+      m_defaultFont = 'bold 16px sans-serif',


should we use font size 12? as default which I have seen is more common but perhaps 12 is too small?

I picked bold 16px sans-serif because I thought it was a good default for the annotations. Generally, I think a text feature will be used for labels not blocks of text, so a larger size is more likely to be appropriate. I could be persuaded otherwise.

manthey · 2017-07-21T12:41:24Z

Do we want showLabels to default to true? If so, anything that currently uses annotations that switches to the new code will suddenly have labels, which could either be good or bad depending on what that program expects.

Add a pug linting test.

aashish24 · 2017-07-20T22:04:40Z

src/canvas/textFeature.js

+    /* If any of the font styles other than `font` have values, then we need to
+     * construct a single font value from the subvalues.  Otherwise, we can
+     * skip it. */
+    fontFromSubValues = ['fontStyle', 'fontVariant', 'fontWeight', 'fontStretch', 'fontSize', 'lineHeight', 'fontFamily'].some(function (key) {


worth wrapping the line?

aashish24 · 2017-07-26T14:59:10Z

examples/annotations/main.css

@@ -4,7 +4,7 @@
  position: absolute;
  left: 10px;
  top: 80px;
-  z-index: 1;
+  z-index: 1000;


I might know the answer..but any more insight on why this change might be useful.

When layers are children of other layers, we set a z-index on them. When we have multiple layers, this conflicts with the z-index context of the controls. The easy solution is to move the controls to an arbitrary high z-index. It might be better to ensure that the map is a stacking context, which on some browsers would be adding css isolation: isolate, but that doesn't work everywhere and other methods of creating a stacking context involve altering css properties on the map div which might be undesirable in general (we could do so in the examples, though).

aashish24 · 2017-07-26T15:08:39Z

src/annotation.js

@@ -1229,18 +1444,33 @@ lineRequiredFeatures[lineFeature.capabilities.basic] = [annotationState.create];
 registerAnnotation('line', lineAnnotation, lineRequiredFeatures);

 /**
- * Point annotation class
+ * Point annotation classa.


classa? --> class?

aashish24 · 2017-07-26T15:14:54Z

src/annotationLayer.js

- *    is treated as a double-click when working with annotations.
- * @param {number} [options.adjacentPointProximity=5]  The minimum distance in
+ * @param {object} [args] Layer options.
+ * @param {number} [args.dblClickTime=300] The delay in milliseconds that is


I am wondering if 300ms is longer than usual but this is not something for this PR

iOS uses 300 ms, Windows uses 500 ms.

good to know, thanks

aashish24

👍

Switch from jade to pug.

manthey added 10 commits July 6, 2017 15:17

Set text fonts.

1947886

This handles font substyles, such as fontSize. This is exposed in the annotations example. This also handles textAlign, textBaseline, color, and textOpacity. Much of the infrastructure for other text styles is in place.

Add text opacity support.

de61c62

This adds a utility function to combine a color with an optional opacity and an opacity value into a single color with opacity value.

Merge branch 'master' into annotation-labels

62a6d7e

Accept css color names in any case.

56da1e0

Better handle blanks in the annotations example. Show a show label option in the annotations example.

Support text rotation, scale, and offset.

07ec8c9

Support text shadow and stroke.

1874547

Add tests for the new utility code.

83feaab

Add tests for textFeature.

b95616c

Remove the direction property -- it isn't supported by Firefox and only obscurely supported by Chrome.

Add tests for annotation labels.

a22213c

aashish24 reviewed Jul 19, 2017

View reviewed changes

aashish24 reviewed Jul 20, 2017

View reviewed changes

src/textFeature.js

@@ -0,0 +1,207 @@

var inherit = require('./inherit');

Copy link

Member

aashish24 Jul 20, 2017

Choose a reason for hiding this comment

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

the API is nice @manthey

aashish24 reviewed Jul 20, 2017

View reviewed changes

Switch from jade to pug.

5ab01dd

Add a pug linting test.

aashish24 reviewed Jul 26, 2017

View reviewed changes

Fix a typo in a comment. Wrap a long line.

6125e08

aashish24 approved these changes Jul 26, 2017

View reviewed changes

Merge pull request #722 from OpenGeoscience/jade-to-pug

790a6b9

Switch from jade to pug.

manthey merged commit 6798308 into master Jul 26, 2017

manthey deleted the annotation-labels branch July 26, 2017 16:06

manthey mentioned this pull request Jul 26, 2017

Add ability to append text to featues #352

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotation labels #719

Annotation labels #719

manthey commented Jul 17, 2017

aashish24 left a comment

aashish24 Jul 20, 2017

aashish24 Jul 20, 2017

manthey Jul 21, 2017

manthey commented Jul 21, 2017

aashish24 Jul 20, 2017

manthey Jul 26, 2017

aashish24 Jul 26, 2017

manthey Jul 26, 2017

aashish24 Jul 26, 2017

manthey Jul 26, 2017

aashish24 Jul 26, 2017

manthey Jul 26, 2017

aashish24 Jul 26, 2017

aashish24 left a comment

Annotation labels #719

Annotation labels #719

Conversation

manthey commented Jul 17, 2017

aashish24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manthey commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aashish24 left a comment

Choose a reason for hiding this comment