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

Add memberObj in @at event #50

Merged
merged 6 commits into from
Mar 28, 2018
Merged

Add memberObj in @at event #50

merged 6 commits into from
Mar 28, 2018

Conversation

goldsteinr
Copy link

I added the possibility to get the member's array object instead of just the text.

Example: I want a user email and not only his name. With the handler now, we get the member obj to use any information we want.
closes #49

Copy link
Owner

@fritx fritx left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, but needs more improvement as I commented 😃

src/At.vue Outdated
return i;
}
});
this.$emit('at', memberObj);
Copy link
Owner

Choose a reason for hiding this comment

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

It would cause a breaking change which we should try to avoid 😃

@@ -17,6 +17,7 @@
<ul class="atwho-ul">
<li v-for="(item, index) in atwho.list"
class="atwho-li"
:key="index"
Copy link
Owner

Choose a reason for hiding this comment

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

👍 we do need a :key in v-for

Copy link
Author

Choose a reason for hiding this comment

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

👍

src/At.vue Outdated
@@ -255,7 +255,12 @@ export default {
} else {
const { members, filterMatch, itemName } = this
if (!keep && chunk.length>0) {
this.$emit('at', chunk)
const memberObj = Object.values(this.members).filter((i) => {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm worried about cross-browser compatibility of Object.values (not supported in IE, etc)
https://caniuse.com/#search=object.values

@goldsteinr
Copy link
Author

@fritx I updated the code with the requested changes!

src/At.vue Outdated
}
});
this.$emit('at', memberObj);
this.$emit('at', memberObj)
Copy link
Owner

Choose a reason for hiding this comment

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

Great, but I had thought what if we $emit('at', chunk, memberObj)

the memberObj as the third arg... lol

Copy link
Owner

Choose a reason for hiding this comment

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

so that we could let go the new added prop showMemberObj

Copy link
Author

Choose a reason for hiding this comment

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

gonna update now

@goldsteinr
Copy link
Author

goldsteinr commented Mar 27, 2018 via email

@goldsteinr
Copy link
Author

@fritx just add the third argument.

@fritx fritx merged commit 9184b33 into fritx:dev Mar 28, 2018
fritx added a commit that referenced this pull request Mar 28, 2018
@at events were always broken, need to be fixed
@fritx
Copy link
Owner

fritx commented Mar 28, 2018

@goldsteinr thanks for the PR, but I made some further changes:

  • kept explicit :key="index" (need custom though), https://vuejs.org/v2/guide/list.html#key
  • @at events were broken
    • inconsistent between At.vue and AtTextarea.vue
    • emitted with trailing space when item inserted
  • so I removed it and introduced @insert events
<at @insert="handleInsert"></at>

methods: {
  handleInsert (item) { /* ... */ }
}

@goldsteinr goldsteinr deleted the feature-at-handle branch March 28, 2018 16:26
@goldsteinr
Copy link
Author

goldsteinr commented Mar 28, 2018

@fritx cool :) can we get a release of it?

@fritx fritx self-assigned this Mar 29, 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.

Is there a way to handle a change?
2 participants