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

feat(NcActionInput): allow to append NcSelect to body #5017

Merged
merged 1 commit into from Jan 5, 2024

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Jan 1, 2024

☑️ Resolves

  • This allows to append the NcSelect in NcActionInput to body. This prevents clipping the select by the action menu.

Code-wise it would be simpler to just remove :append-to-body="false", but this would introduce a breaking change, as it would set append-to-body to true by default for NcSelect. So I decided to add a prop instead.
Also, for vue 3 this is not necessary, since v-bind="$attrs" overwrites the props already set on the component.

🖼️ Screenshots

🏚️ Before 🏡 After
Bildschirmfoto vom 2024-01-01 12-39-14 Bildschirmfoto vom 2024-01-01 12-40-09

@raimund-schluessler raimund-schluessler added enhancement New feature or request 3. to review Waiting for reviews feature: actions Related to the actions components feature: select Related to the NcSelect* components labels Jan 1, 2024
@raimund-schluessler raimund-schluessler added this to the 8.5.0 milestone Jan 1, 2024
@raimund-schluessler raimund-schluessler marked this pull request as ready for review January 1, 2024 11:43
@ShGKme
Copy link
Contributor

ShGKme commented Jan 3, 2024

Also, for vue 3 this is not necessary, since v-bind="$attrs" overwrites the props already set on the component.

Then an alternative solution could be to emulate Vue 3 behavior:

v-bind="{ appendToBody: false, ...$attrs }"

It would allow avoiding adding a specific prop for NcSelect

Copy link

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Great fix :) As @marcoambrosini has mentioned we should generally move away from nesting components in an ActionMenu though :)

@raimund-schluessler
Copy link
Contributor Author

v-bind="{ appendToBody: false, ...$attrs }"


It would allow avoiding adding a specific prop for `NcSelect`

I think this is the best solution, as it saves us from adding a prop. I changed it accordingly. It just has a tiny flaw, that one has to correctly name the attribute, as only append-to-body but not appendToBody will work.

@raimund-schluessler
Copy link
Contributor Author

raimund-schluessler commented Jan 4, 2024

@nimishavijay You seem to have no write access to the repository, so I cannot merge yet 🙂 Is this on purpose, or should you be granted access by someone who can?

@raimund-schluessler raimund-schluessler changed the title feat(NcActionInput): allow to append NcSelect to body feat(NcActionInput): allow to append NcSelect to body Jan 4, 2024
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler raimund-schluessler merged commit d9f9ed4 into master Jan 5, 2024
15 checks passed
@raimund-schluessler raimund-schluessler deleted the fix/noid/action-input-append-body branch January 5, 2024 08:02
@Pytal Pytal mentioned this pull request Jan 23, 2024
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 enhancement New feature or request feature: actions Related to the actions components feature: select Related to the NcSelect* components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants