-
Notifications
You must be signed in to change notification settings - Fork 27.4k
docs(guide/Components): Added missed fieldType integration #14059
Conversation
In the file `name="editableField.html"` was missed the `fieldType` property integration.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
1 similar comment
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
1 similar comment
CLAs look good, thanks! |
@@ -282,7 +282,7 @@ it upwards to the heroList component, which updates the original data. | |||
</file> | |||
<file name="editableField.html"> | |||
<span ng-switch="$ctrl.editMode"> | |||
<input ng-switch-when="true" type="text" ng-model="$ctrl.fieldValue"> | |||
<input ng-switch-when="true" type="{{::$ctrl.fieldType}}" ng-model="$ctrl.fieldValue"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove the ::
(so we don't introduce another concept and because there is nothing preventing the fieldType
to change dynamically 😃)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is not it the best practice for performance?
I mean in the example the fieldType
is not changed dynamically so it makes it more transparent, what is wanted to be achieved.
Finally, for the ::
I can add a note, with link to read about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is not it the best practice for performance?
It is, but only when the usecase allows it. This is a very simple demo, of course, but in a real app, for the generic, reusable editableField
component you'd probably want to allow consumers to change the field type dynamically.
In any case, the main purpose of this guide is to showcase components and their usage and related use-patterns and to that end I believe it's best to keep other concepts at a minimum. I wouldn't oppose, if ::
was aready used elsewhere (e.g. I'm sure there are places in this guide where it is much more appropriate - but this is not the point), but using it here unnecessarily introduces yet another concept.
So, I would prefer it gone (but it's not super important, so feel free to leave it as is if you feel strongly about it 😃)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahaha, thanks! I will remove it. Because I like to keep everything clean and easy :)
Backported to |
In the file
name="editableField.html"
was missed thefieldType
property integration.