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

docs(tutorial/6 - Two-way Data Binding): fix exp. documentation #16781

Closed
wants to merge 2 commits into from

Conversation

anthonyx
Copy link
Contributor

@anthonyx anthonyx commented Dec 3, 2018

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Documentation update in step 6 of the PhoneCat Tutorial for AngularJS

What is the current behavior? (You can also link to an open issue here)

In the Experiments section of step 6 in AngularJS Tutorial, it is suggested to add a - symbol to <option value="age">Oldest</option>

This change is made to reverse the sort order when selecting the age option.

However, this change affects the default $ctrl.orderProp that we had set in app/phone-list/phone-list.component.js

After making the change, our default "Sort by" option is blank

What is the new behavior (if this is a feature change)?

I added additional documentation to clarify that this behavior makes sense and that the reader should try and correct the incorrect default behavior within app/phone-list/phone-list.component.js

Does this PR introduce a breaking change?

No

Let me know if this makes sense or if there's anything I can do to help improve the fix.
Thank you!

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • [ ] Fix/Feature: Docs have been added/updated
  • [ ] Fix/Feature: Tests have been added; existing tests pass

Other information:

In the experiments section, it is suggested to add a `-` symbol to `<option value="age">Oldest</option>`
This change is made to reverse the sort order when selecting the "age" option.

However, this change affects the default $ctrl.orderProp that we had set in app/phone-list/phone-list.component.js
After making the change, our default when refreshing the page is "Sort by: [blank]"

I added some additional documentation to clarify that this behavior makes sense and that the reader should try and fix this within phone-list.component.js

Let me know if this makes sense or if there's anything I can do to help improve the fix.
Thank you!
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

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 (if not, look below for help). 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 (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@anthonyx
Copy link
Contributor Author

anthonyx commented Dec 3, 2018

I signed it! Signed the CLA

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Dec 3, 2018
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Good catch 👍
I have left some minor comments.

@@ -230,6 +230,9 @@ You can now rerun `npm run protractor` to see the tests run.

* Reverse the sort order by adding a `-` symbol before the sorting value:
`<option value="-age">Oldest</option>`
Upon refreshing the page, you'll see that the default selection of the "age" option in "Sort by" is blank again.
Try fixing this in **`app/phone-list/phone-list.component.js`:**

Copy link
Member

Choose a reason for hiding this comment

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

🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, deleted the extra line. Fixed in 41e28b6

@@ -230,6 +230,9 @@ You can now rerun `npm run protractor` to see the tests run.

* Reverse the sort order by adding a `-` symbol before the sorting value:
`<option value="-age">Oldest</option>`
Upon refreshing the page, you'll see that the default selection of the "age" option in "Sort by" is blank again.
Try fixing this in **`app/phone-list/phone-list.component.js`:**
Copy link
Member

Choose a reason for hiding this comment

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

There is a redundand : after the filename and the . is missing at the end of the sentence.
I would also change wrap the filename in ` instead of ** for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've deleted the redundant : and also added the . at the end of the sentence. Fixed the filename wrap as well. Fixed in 41e28b6

@@ -230,6 +230,9 @@ You can now rerun `npm run protractor` to see the tests run.

* Reverse the sort order by adding a `-` symbol before the sorting value:
`<option value="-age">Oldest</option>`
Upon refreshing the page, you'll see that the default selection of the "age" option in "Sort by" is blank again.
Copy link
Member

Choose a reason for hiding this comment

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

I would rephrase this to something like:

You'll notice that the default ordering by age does not work any more and the drop-down list has a blank option selected.
Fix this, by updating the orderProp value in phone-list.component.js to match the new value on the <option> element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the documentation to more closely follow your suggested change. Thank you for improving the clarity! Fixed in 41e28b6

@anthonyx anthonyx changed the title docs(tutorial/6 - Two-way Data Binding): Fixed Experiments Documentation docs(tutorial/6 - Two-way Data Binding): fix exp. documentation Dec 5, 2018
gkalpak pushed a commit that referenced this pull request Dec 5, 2018
In the experiments section, it is suggested to add a `-` symbol to
`<option value="age">Oldest</option>`. This change is made to reverse
the sort order when selecting the `age` option.

However, this change affects the default `$ctrl.orderProp` that we had
set in `phone-list.component.js`. After making the change, our default
when refreshing the page is "Sort by: [blank]".

This commit adds some additional documentation to clarify that this
behavior makes sense and that the reader should try and fix this within
`phone-list.component.js`.

Closes #16781
@gkalpak gkalpak closed this in fa8fe1f Dec 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants