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

GMF Print: Move from directive to component #3155

Merged
merged 1 commit into from
Nov 23, 2017

Conversation

ger-benjamin
Copy link
Member

@ger-benjamin ger-benjamin commented Nov 23, 2017

For GSGMF-155

Work well on example, and apps

@ger-benjamin ger-benjamin force-pushed the gmfprint_directive_to_component branch 2 times, most recently from 727596b to 7f74fc8 Compare November 23, 2017 09:34
@@ -110,9 +110,9 @@
<a class="btn close" ng-click="mainCtrl.printPanelActive = false">&times;</a>
</div>
<gmf-print
gmf-print-map="mainCtrl.map"
gmf-print-map="::mainCtrl.map"
Copy link

Choose a reason for hiding this comment

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

no real need.. gmf-print should use one-way binding

Copy link
Member Author

@ger-benjamin ger-benjamin Nov 23, 2017

Choose a reason for hiding this comment

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

That's not the same thing.
'<' stand for "one-WAY" bindings.
& (in js) or :: (in html) stand for "one-TIME" binding.
I think the more important is the one-time binding for the map and with FredJ, we find that it's more practical to use the html notation.

Copy link

Choose a reason for hiding this comment

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

I told to use one-way because you are passing a map object, so profit is not so big.

As for me it makes sense only for some data that will never change like captions and(or) calculated expressions

https://docs.angularjs.org/guide/expression#reasons-for-using-one-time-binding

/**
* Init the controller
*/
gmf.PrintController.prototype.$onInit = function() {
Copy link

Choose a reason for hiding this comment

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

you can use classes

Copy link
Member Author

@ger-benjamin ger-benjamin Nov 23, 2017

Choose a reason for hiding this comment

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

I think we should do that in another PR (that already a big one and all other component still use this notation)

*/
this.currentThemes_;
const getScaleFn = (frameState) => {
Copy link

Choose a reason for hiding this comment

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

why not a method? it seems a bit complex arrow-function

Copy link
Member Author

@ger-benjamin ger-benjamin Nov 23, 2017

Choose a reason for hiding this comment

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

I agree, but that's minor (was a moved code part) and I'm not here tomorrow, if I don't merge now, I'll merge Monday :-/

@ger-benjamin ger-benjamin merged commit ab37bd0 into master Nov 23, 2017
@ger-benjamin ger-benjamin deleted the gmfprint_directive_to_component branch November 23, 2017 15:44
@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.

2 participants