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

[next] fix(NcActionInput): listen to correct events #5231

Merged
merged 5 commits into from
Feb 20, 2024

Conversation

raimund-schluessler
Copy link
Contributor

☑️ Resolves

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler raimund-schluessler added this to the 9.0.0-alpha.2 milestone Feb 9, 2024
@raimund-schluessler raimund-schluessler linked an issue Feb 9, 2024 that may be closed by this pull request
@raimund-schluessler raimund-schluessler changed the title fix(NcActionInput): list to correct events [next] fix(NcActionInput): list to correct events Feb 9, 2024
@raimund-schluessler raimund-schluessler added bug Something isn't working 3. to review Waiting for reviews feature: actions Related to the actions components vue 3 Related to the vue 3 migration labels Feb 9, 2024
@raimund-schluessler raimund-schluessler marked this pull request as ready for review February 9, 2024 09:51
@raimund-schluessler raimund-schluessler changed the title [next] fix(NcActionInput): list to correct events [next] fix(NcActionInput): listen to correct events Feb 9, 2024
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Makes sense.

I haven't tested, but it looks like input and change events doesn't work anymore with this PR. As they only proxied the same native events with native Event payload, we probably can just remove them from the emits declaration to bring them back. (Will work as a part of v-bind="$attrs")

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler
Copy link
Contributor Author

Makes sense.

I haven't tested, but it looks like input and change events doesn't work anymore with this PR. As they only proxied the same native events with native Event payload, we probably can just remove them from the emits declaration to bring them back. (Will work as a part of v-bind="$attrs")

@ShGKme I removed them from the emits declaration and @change and @input are emitted now. Unfortunately, they are emitted twice, do you have an idea why? Could this be a problem with NcTextField?

@ShGKme
Copy link
Contributor

ShGKme commented Feb 19, 2024

@ShGKme I removed them from the emits declaration and @change and @input are emitted now. Unfortunately, they are emitted twice, do you have an idea why? Could this be a problem with NcTextField?

Component is missing inheritAttrs: false.

Once event listener is set by v-bind="$attrs" on the input element itself, and once on the root element <li> by the default inheritance (input and change events bubble).

Probably, we should add vue/no-duplicate-attr-inheritance, I saw this issue in many places already...

@raimund-schluessler
Copy link
Contributor Author

Setting inheritAttrs: false for NcActionInput might then break things we want, like style or css no?

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler
Copy link
Contributor Author

@ShGKme I added inheritAttrs and it fixes the double call to the handler. I guess if we notice, we need css and style we will have to add it manually later.

@ShGKme
Copy link
Contributor

ShGKme commented Feb 19, 2024

Setting inheritAttrs: false for NcActionInput might then break things we want, like style or css no?

Yes, but using v-bind="$attrs" without inheritAttrs: false is always an issue, it duplicates everything. We can add class to props as we did in other places to set it on the root.

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler
Copy link
Contributor Author

Setting inheritAttrs: false for NcActionInput might then break things we want, like style or css no?

Yes, but using v-bind="$attrs" without inheritAttrs: false is always an issue, it duplicates everything. We can add class to props as we did in other places to set it on the root.

Done.

@raimund-schluessler raimund-schluessler merged commit 383abe0 into next Feb 20, 2024
17 checks passed
@raimund-schluessler raimund-schluessler deleted the fix/5229/action-input-events branch February 20, 2024 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working feature: actions Related to the actions components vue 3 Related to the vue 3 migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[next] NcActionInput does not listen to correct events
4 participants