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

chore(Vue3): v-bind checking #12005

Merged
merged 1 commit into from
Apr 5, 2024
Merged

chore(Vue3): v-bind checking #12005

merged 1 commit into from
Apr 5, 2024

Conversation

DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented Apr 4, 2024

☑️ Resolves

  • Lowered v-bind to keep the same behavior in Vue 2 when migrating to Vue 3
  • BreakoutRoomsEditor has v-bind="$attrs" on its root element, not needed as it is inherited automatically (in Vue 3) but also no props are needed from $attrs-> removed
  • Checked components where their $attrs has been used. If there are classes or styles that might be forwarded ( in Vue 3, classes and styles are also included) -> none
  • Found an icon for speaker view in Material Icon, so I just replaced the current one.

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After
Screenshot before image

🚧 Tasks

  • Another scan to double-check ?

🏁 Checklist

  • 🌏 Tested with Chrome, Firefox and Safari or should not be risky to browser differences
  • 🖥️ Tested with Desktop client or should not be risky for it
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible
  • 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required

@DorraJaouad DorraJaouad added this to the 💙 Next Major (30) milestone Apr 4, 2024
@DorraJaouad DorraJaouad requested a review from ShGKme April 4, 2024 13:02
@DorraJaouad DorraJaouad self-assigned this Apr 4, 2024
@DorraJaouad DorraJaouad force-pushed the chore/noid/check-binding branch from 1295e86 to 2d3838e Compare April 4, 2024 13:10
@DorraJaouad DorraJaouad mentioned this pull request Apr 4, 2024
47 tasks
ShGKme
ShGKme previously approved these changes Apr 4, 2024
@ShGKme ShGKme dismissed their stale review April 4, 2024 16:23

Missed one thing

<NcModal v-bind="$attrs"
:container="container"
<NcModal :container="container"
Copy link
Contributor

Choose a reason for hiding this comment

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

BreakoutRoomsEditor has v-bind="$attrs" on its root element, not needed as it is inherited automatically -> removed

This is not completely correct.

In Vue 3 - yes. Attribute inheritance works for both, HTML attributes and component props.

However, in Vue 2 it doesn't. It inherit attributes only as attributes to put them into $attrs or the HTML element itself. But not as component props.

So, if we remove v-bind here, no BreakoutRoomsEditor attributes will be passed NcModal's props.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still, it is fine to be removed.

Not because "it is inherited automatically" but because this BreakoutRoomsEditor component actually doesn't need this inheritance at all. Currently it is never used with NcModal props.

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, I also checked and saw no props are needed from the component $attrs but forgot to mention it 😄

…ribute selection in vue 2 (individual attributes are always selected over those in v-bind). In vue 3, it depends on the order.

Signed-off-by: DorraJaouad <dorra.jaoued7@gmail.com>
@DorraJaouad DorraJaouad force-pushed the chore/noid/check-binding branch from 2d3838e to 3d67844 Compare April 4, 2024 21:07
@DorraJaouad
Copy link
Contributor Author

rebased onto main

@DorraJaouad DorraJaouad merged commit fc49660 into main Apr 5, 2024
46 checks passed
@DorraJaouad DorraJaouad deleted the chore/noid/check-binding branch April 5, 2024 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants