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

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

Merged
merged 3 commits into from
Oct 21, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Oct 18, 2023

☑️ 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
@szaimen

This comment was marked as resolved.

@szaimen szaimen removed their request for review October 19, 2023 09:22
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

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
Comment on lines -152 to -154
helperText: {
type: String,
default: '',
Copy link
Contributor Author

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

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@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 susnux and Pytal and removed request for susnux and Pytal October 21, 2023 00:11
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
15 checks passed
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants