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: Input attrs not reactive #228

Merged
merged 5 commits into from
Jan 2, 2024

Conversation

ParasSolanki
Copy link
Contributor

Fixes: #224

@ParasSolanki
Copy link
Contributor Author

Do i have to run pnpm build:registry command?
I am using Windows laptop so seeing inconsistency in registry output. #181

@sadeghbarati
Copy link
Collaborator

No, no need to build the registry on your end

@sadeghbarati
Copy link
Collaborator

I have thought about props and attrs. Please share your thoughts, too 🙏


attrs way

I think making read-only Proxy (useAttrs, $attrs) reactive is not a good idea

We are using computed or maybe toRefs for attrs to make them reactive/can-be-mutated inside the Component, which they can't, we are changing their nature (readonly)


props way

By adding class to props, class will be removed from $attrs object so we will not get duplicated class so we can use props.class in cn function and v-bind="$attrs" without worrying about duplication

Even though we are not changing/mutating some props like class (toRefs/computed) inside the Component

I think using props is a better way.

Sorry for my bad English. I'll try to make a Vue SFC Playground example to explain what I meant

#35

@ParasSolanki
Copy link
Contributor Author

Yeah i also think using Props is a better approach here. Created a playground for it.

Vue SFC Playground

Sorry for the late response.

@sadeghbarati
Copy link
Collaborator

Just changed one thing in your playground, also thanks for your comment

Vue SFC Playground

Let's fake imagine class type changes in the feature so using this type from Vue (HTMLAttributes['class']) will save us refactoring tons of Components Props

- class?: string
+ class?: HTMLAttributes['class']

@ParasSolanki
Copy link
Contributor Author

Yeah this is right way to do the typing. I should've used the HTMLAttributes['class'].

@ParasSolanki
Copy link
Contributor Author

Now, Should i change my implementation to using Props for class? Or there are still some scenarios or edge cases we want to check first?

@sadeghbarati
Copy link
Collaborator

Yes, go with Props

Please, if you have time, refactor all the components

@ParasSolanki
Copy link
Contributor Author

Yeah, okay.
And by all you mean FormItem, PaginationEllipsis and other components where we are using useAttrs for class?

@sadeghbarati
Copy link
Collaborator

Yes, only useAttrs 🙏

I'll refactor the rest if it's needed

@ParasSolanki
Copy link
Contributor Author

Change the implementation to using props for class. Let me know if there are any other issue.

@sadeghbarati sadeghbarati merged commit 0f0f1ef into unovue:dev Jan 2, 2024
1 check failed
@ParasSolanki ParasSolanki deleted the fix/input-props-not-reactive branch January 30, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Input attrs are not reactive
2 participants