Skip to content

Comments

fix(Nc*Field): do not pass all props to InputField BY filtering $props#4666

Merged
Pytal merged 3 commits intomasterfrom
fix/text-field--do-not-pass-extra-props-2
Oct 21, 2023
Merged

fix(Nc*Field): do not pass all props to InputField BY filtering $props#4666
Pytal merged 3 commits intomasterfrom
fix/text-field--do-not-pass-extra-props-2

Conversation

@ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Oct 18, 2023 •
edited
Loading

☑️ Resolves

HTML Validation issue.

Currently NcTextField passes both $attrs and $props to the NcInputField.

<NcInputField v-bind="{ ...$attrs, ...$props }">

While passing $attrs makes sense, passing $props is valid only when both components have exactly the same props. But they don't. NcTextField has its own prop trailingButtonIcon and may have more props in the future.

Proposal: same as NcSelect.

Filter props to proxy only original NcInputField props.

Alternative solution: do not use props for proxying.

See: #4665

🖼️ Screenshots

trailingbuttonicon is not a valid <input> attribute.

image

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@ShGKme ShGKme added bug Something isn't working accessibility Making sure we design for the widest range of people possible, including those who have disabilities feature: input-field Covering the InputField, TextField, ... labels Oct 18, 2023
@ShGKme ShGKme added this to the 8.0.0 milestone Oct 18, 2023
@ShGKme ShGKme self-assigned this Oct 18, 2023
@ShGKme ShGKme mentioned this pull request Oct 18, 2023
Closed
2 tasks
@szaimen

This comment was marked as resolved.

Sign in to view
@szaimen szaimen removed their request for review October 19, 2023 09:22
ChristophWurst
ChristophWurst approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@ChristophWurst ChristophWurst 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

@ShGKme
Copy link
Contributor Author

ShGKme commented Oct 19, 2023 •
edited
Loading

makes sense

Thanks! Could you also have a look at this PR with an alternative (more preferred by me to be honest): #4665

@ShGKme ShGKme force-pushed the fix/text-field--do-not-pass-extra-props-2 branch 2 times, most recently from 5cd3e2f to 94968e5 Compare October 20, 2023 23:42
@ShGKme
Copy link
Contributor Author

ShGKme commented Oct 21, 2023

Although an alternative approach with a classic transparent wrapper approach (with proxying only $attrs) seems more clean (and performant) for me and @susnux, I checked how IDEs handle the component, how Vue 3.3 works now with new implementation of defineProps<WrappedProps & NewProps>, discussed it with some experts and decided that this PR approach is better in terms of DX.

Even with Vue 3.3 + SFC Setup + TS it is still not possible to add typing for attributes for wrapper components.

Waiting for Vue to add something for defining attrs types for component wrappers 😭

@ShGKme ShGKme marked this pull request as ready for review October 21, 2023 00:05
@ShGKme ShGKme changed the title fix(NcTextField): do not pass all props to InputField BY filtering $props fix(Nc*Field): do not pass all props to InputField BY filtering $props Oct 21, 2023
ShGKme
ShGKme commented Oct 21, 2023
View reviewed changes
src/components/NcPasswordField/NcPasswordField.vue
Comment on lines -152 to -154
helperText: {
type: String,
default: '',
Copy link
Contributor Author

@ShGKme ShGKme Oct 21, 2023

Choose a reason for hiding this comment

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

helperText removed, because it is exactly the same as in NcInputField and already defined

@ShGKme ShGKme force-pushed the fix/text-field--do-not-pass-extra-props-2 branch from 94968e5 to a3f48e3 Compare October 21, 2023 00:09
@ShGKme ShGKme requested review from Pytal and susnux and removed request for Pytal and susnux October 21, 2023 00:11
susnux
susnux approved these changes Oct 21, 2023
View reviewed changes
Copy link
Contributor

@susnux susnux 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, even if I think the other PR is cleaner 😞

@ShGKme
Copy link
Contributor Author

ShGKme commented Oct 21, 2023

Makes sense, even if I think the other PR is cleaner 😞

Clean, fast, without any auto-complete in IDE :(

@Pytal Pytal merged commit 421295b into master Oct 21, 2023
@Pytal Pytal deleted the fix/text-field--do-not-pass-extra-props-2 branch October 21, 2023 00:21
@Pytal Pytal mentioned this pull request Oct 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@susnux susnux susnux approved these changes

@raimund-schluessler raimund-schluessler Awaiting requested review from raimund-schluessler

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

+1 more reviewer

@Pytal Pytal Pytal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ShGKme ShGKme

Labels

accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: input-field Covering the InputField, TextField, ...

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ShGKme @szaimen @ChristophWurst @susnux @Pytal