Skip to content

Document changes related to $attrs $listeners and v-on.native (close #526,#592) #608

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Oct 23, 2020
Merged

Document changes related to $attrs $listeners and v-on.native (close #526,#592) #608

merged 21 commits into from
Oct 23, 2020

Conversation

LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Oct 7, 2020 •
edited
Loading

Description

This PR adds migration guidance for the following changes:

  • $attrs not included class and style attributes
  • $listeners removed & merged with $attrs
  • v-on.native modifier removed
  • New: declare emitted event with the new emits option

Remarks

  • Please review wording and possible typos
  • I'm not sure about the placement of these topics in the the listing on the "Introduction" place. These topics are all interrelated more or less, but I didn't find a good topic to place them under together.

close: #526
close: #592

Thorsten Luenborg added 2 commits October 7, 2020 08:43
feat: Add migration guide for 3 changes:
04f80cb 
- $listeners removed
- v-on.native modifier removed
- new emits option
feat: document $attrs new behaviour
426b551 
now includes class and style behaviour.
skirtles-code
skirtles-code suggested changes Oct 8, 2020
View reviewed changes
Copy link
Contributor

@skirtles-code skirtles-code left a comment

Choose a reason for hiding this comment

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

Some feedback from me.

Some of my comments are very brief. I hope they don't come across as curt, I was just trying to get you feedback as quickly as possible.

Happy to clarify if anything I've said isn't clear.

src/guide/migration/emits-option.md Outdated

Even though this is the new optional feature, it is highly recommended that you document all of the emitted events of your components this way because of the [removal of the `.native` modifier](./native-modifier-removed.md).

All events not defined with `emits` are now added as DOM event listeners to the components root node (unless `inheritAttrs: false` has been set).
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe component's needs an apostrophe.

Copy link
Member

Choose a reason for hiding this comment

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

I understand what you mean, but that's not grammatically correct in English, unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sdras. This sentence seems to be grammatically correct to me, aside from the missing apostrophe. It does need the reader to parse it the correct way though, so maybe it should be reworded to make it easier to follow?

Copy link
Member Author

Choose a reason for hiding this comment

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

The first paragraph speaks about "components", plural. I'm with Sarah that we can't switch to singular in the next paragraph. But doesn't "components'" exist in English? No sure ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to make it plural is going to be tricky as root node is singular. I think the components' root nodes would be confusing as it could be misconstrued as a reference to fragments.

Personally, I'd try to make the component singular.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did a little change in wording of the previous sentence to make singular work better.

I think it's no at least grammatically correct and gets the point across.

@LinusBorg LinusBorg changed the title Document changes related to $attrs $listeners and v-on:native (close #526,#592) Document changes related to $attrs $listeners and v-on.native (close #526,#592) Oct 9, 2020
skirtles-code
skirtles-code suggested changes Oct 10, 2020
View reviewed changes
Copy link
Contributor

@skirtles-code skirtles-code left a comment

Choose a reason for hiding this comment

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

A few more thoughts from me.

It looks like a lot but it's mostly just broken links.

LinusBorg and others added 10 commits October 10, 2020 10:40
@LinusBorg
Copy link
Member Author

@skirtles-code thanks for this thorough review!

@skirtles-code
Copy link
Contributor

@LinusBorg Could you let me know when you're done making changes so I can go through it again?

There are still quite a few of my previous comments that haven't been addressed. I'm not sure if that's because you haven't got round to them yet or if you just didn't see them. GitHub is hiding them behind Load more links (both reviews but especially the second one).

Thanks.

@LinusBorg
Copy link
Member Author

LinusBorg commented Oct 11, 2020 •
edited
Loading

I'm done for now, fixed the additional, hidden things.

skirtles-code
skirtles-code approved these changes Oct 12, 2020
View reviewed changes
Copy link
Contributor

@skirtles-code skirtles-code left a comment

Choose a reason for hiding this comment

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

A couple of final suggestions from me.

I'm happy for this to be merged.

@LinusBorg
Copy link
Member Author

Hope I got everything covered now. Would be good if can merge this soon as this is pretty vital info missing from the guide right now.

If we want to fix wording here and there or spot more minor grammar mistakes we can do that in another PR, WDYT?

skirtles-code
skirtles-code reviewed Oct 13, 2020
View reviewed changes
src/guide/migration/emits-option.md

## Migration Strategy

It is highly recommended that you document all of the emitted events by your each of components this way because of the [removal of the `.native` modifier](./v-on-native-modifier-removed.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

The words are not quite in the right order here. I think you were aiming for:

document all of the events emitted by each of your components

@skirtles-code
Copy link
Contributor

@LinusBorg I'm inclined to agree. This is important stuff and it needs to be in the guide.

@skirtles-code
Copy link
Contributor

@NataliaTepluhina Can this be merged?

@NataliaTepluhina NataliaTepluhina merged commit d3af36c into master Oct 23, 2020
nick-lai pushed a commit to nick-lai/docs-next that referenced this pull request Dec 2, 2020
@LinusBorg @skirtles-code @nick-lai
Document changes related to $attrs $listeners and v-on.native (close v…
3b1885e 
…uejs#526,vuejs#592) (vuejs#608)

* feat: Add migration guide for 3 changes:
- $listeners removed
- v-on.native modifier removed
- new emits option

* feat: document $attrs new behaviour
now includes class and style behaviour.

* fix: typos, grammar and language improvements

* more fixes

* Update src/guide/migration/emits-option.md

* Update src/guide/migration/introduction.md

fix broken link

* Update src/guide/migration/listeners-removed.md

fix broken link

Co-authored-by: skirtle <[email protected]>

* Update src/guide/migration/introduction.md

fix broken link

Co-authored-by: skirtle <[email protected]>

* Update src/guide/migration/emits-option.md

typo

* Update src/guide/migration/listeners-removed.md

grammar

* Update src/guide/migration/listeners-removed.md

wording

* Update src/guide/migration/v-on-native-modifier-removed.md

* Update src/guide/migration/emits-option.md

fix broken link

* Update src/guide/migration/introduction.md

fix broken link

* still more mistakes to fix.

* fix link to api docs for "emits:"

* use vue instead of html as hightligh lang where possible

* fix broken link

* fix missing template content

* wording/grammar

Co-authored-by: Thorsten Luenborg <[email protected]>
Co-authored-by: skirtle <[email protected]>
@NataliaTepluhina NataliaTepluhina deleted the linusborg/event-listener-chaanges branch February 24, 2021 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdras sdras sdras left review comments

@skirtles-code skirtles-code skirtles-code approved these changes

@bencodezen bencodezen Awaiting requested review from bencodezen

@NataliaTepluhina NataliaTepluhina Awaiting requested review from NataliaTepluhina

@phanan phanan Awaiting requested review from phanan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@LinusBorg @skirtles-code @sdras @NataliaTepluhina