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

Display label drawing #3269

Merged
merged 6 commits into from
Dec 18, 2017
Merged

Display label drawing #3269

merged 6 commits into from
Dec 18, 2017

Conversation

julsbreakdown
Copy link
Member

Enable the user to display the feature label.
As it is done for the measurement label.
cf
capture du 2017-12-13 14-43-17

@julsbreakdown julsbreakdown force-pushed the jwaddle_display_label_drawing branch 4 times, most recently from f16c03b to cd81f45 Compare December 13, 2017 14:22
@julsbreakdown julsbreakdown force-pushed the jwaddle_display_label_drawing branch 4 times, most recently from 1a9fa35 to 77d9790 Compare December 14, 2017 09:06
@julsbreakdown julsbreakdown changed the title [WIP] Jwaddle display label drawing Jwaddle display label drawing Dec 14, 2017
text: this.getNameProperty(feature),
offsetY: -(offset / 2 + 4)
})
}));
Copy link
Member

Choose a reason for hiding this comment

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

@fredj is this offset calculation correct?

Copy link
Member

Choose a reason for hiding this comment

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

no idea

Copy link
Member

Choose a reason for hiding this comment

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

This 25 looks to be a magic number, shouln't we use multiline from OpenLayers: openlayers/openlayers#4512?

text: this.getNameProperty(feature),
offsetY: -(offset / 2 + 4)
})
}));
Copy link
Member

Choose a reason for hiding this comment

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

Same

size: 10,
offsetY: -(offset / 2 + 4)
})
}));
Copy link
Member

Choose a reason for hiding this comment

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

again :-)

@julsbreakdown
Copy link
Member Author

In this case we only have the one style for the label, and use the multiline ('/n' option) if both (measure & label are checked)?

@sbrunner
Copy link
Member

Yes :-)

Copy link
Member

@sbrunner sbrunner left a comment

Choose a reason for hiding this comment

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

Just come code can be simplified (2 times) :-)

if (showLabel && showMeasure){
// display both label using \n
textLabelValue = textProperty + '\n' +textMeasure;
}
Copy link
Member

Choose a reason for hiding this comment

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

We usely do like this:

cost textLabelValues = [];
textLabelValues.push(this.getNameProperty(feature));
textLabelValues.push(this.getMeasure(feature));
cost textLabelValue = textLabelValues.join('\n');

=> shorter and no if :-)

@sbrunner sbrunner changed the title Jwaddle display label drawing Display label drawing Dec 18, 2017
@julsbreakdown julsbreakdown force-pushed the jwaddle_display_label_drawing branch 9 times, most recently from ab841e1 to 448a4c6 Compare December 18, 2017 12:40
@julsbreakdown julsbreakdown changed the base branch from master to 2.2 December 18, 2017 12:47
@julsbreakdown julsbreakdown changed the base branch from 2.2 to master December 18, 2017 12:48
@julsbreakdown julsbreakdown merged commit f35cc27 into master Dec 18, 2017
@julsbreakdown julsbreakdown deleted the jwaddle_display_label_drawing branch December 18, 2017 14:28
@sbrunner sbrunner added this to the 2.3 milestone Apr 12, 2018
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.

None yet

3 participants