Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

docs(guide/Components): Added missed fieldType integration #14059

Closed

Conversation

AlexanderTserkovniy
Copy link
Contributor

In the file name="editableField.html" was missed the fieldType property integration.

In the file `name="editableField.html"` was missed the `fieldType` property integration.
@googlebot
Copy link

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!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment
@googlebot
Copy link

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!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@AlexanderTserkovniy
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

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">
Copy link
Member

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 😃)

Copy link
Contributor Author

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.

Copy link
Member

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 😃)

Copy link
Contributor Author

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 :)

@gkalpak
Copy link
Member

gkalpak commented Feb 16, 2016

Backported to v1.5.x as 2ea7b06.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants