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

Added line annotations. #681

Merged
merged 2 commits into from
Apr 14, 2017
Merged

Added line annotations. #681

merged 2 commits into from
Apr 14, 2017

Conversation

manthey
Copy link
Contributor

@manthey manthey commented Mar 28, 2017

Added line annotations.

Rectangle annotations can be created by clicking opposite points rather than dragging the outline. This allows them to be created on a touch device.

Added style and editstyle convenience functions to annotations -- rather than have to ask for annotation.options('style'), you can do annotation.style().

Fix an issue with the touch handler that produced an offset to the touch event.

Fix an issue in the line feature that didn't recompute the search data when just styles were changed.

Fix an issue with the lines demo where the tooltip wouldn't always show.

Rectangle annotations can be created by clicking opposite points rather than dragging the outline.  This allows them to be created on a touch device.

Added style and editstyle convenience functions to annotations -- rather than have to ask for annotation.options('style'), you can do annotation.style().

Fix an issue with the touch handler that produced an offset to the touch event.

Fix an issue in the line feature that didn't recompute the search data when just styles were changed.

Fix an issue with the lines demo where the tooltip wouldn't always show.
@codecov-io
Copy link

codecov-io commented Mar 28, 2017

Codecov Report

Merging #681 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
+ Coverage   95.14%   95.24%   +0.09%     
==========================================
  Files          83       83              
  Lines        8678     8850     +172     
==========================================
+ Hits         8257     8429     +172     
  Misses        421      421
Impacted Files Coverage Δ
src/action.js 100% <ø> (ø) ⬆️
src/annotation.js 100% <100%> (ø) ⬆️
src/mapInteractor.js 96.11% <100%> (-0.01%) ⬇️
src/lineFeature.js 100% <100%> (ø) ⬆️
src/annotationLayer.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 663a5c4...8104c71. Read the comment docs.

@@ -482,8 +499,6 @@ var annotationLayer = function (args) {
case 'point':
position = [feature.position()(data, data_idx)];
break;
default:
Copy link
Member

Choose a reason for hiding this comment

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

Cannot see in diff.. but i guess we do not need return here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are only three conditions, so we don't need to.

], function (keyidx, key) {
$.each([
'closed', 'fill', 'fillColor', 'fillOpacity', 'line',
'lineCap', 'lineJoin', 'polygon', 'position', 'radius',
Copy link
Member

Choose a reason for hiding this comment

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

should we maintain this list explicitly as some object?

if (evt.pageX === undefined && evt.center !== undefined && evt.center.x !== undefined) {
evt.pageX = evt.center.x + offset.left;
Copy link
Member

Choose a reason for hiding this comment

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

No need for offset now? Or is that included now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We just talked about, but I'll add a note for the historical record: I was mistaken in adding the offset originally, but didn't notice until I added lines. This fixed it.

Copy link
Member

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

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

I tried the example and everything worked as expected.

Copy link
Member

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

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

LGTM

@manthey manthey merged commit c3c4eaf into master Apr 14, 2017
@manthey manthey deleted the lines-annotation branch April 14, 2017 18:53
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.

3 participants