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: add controlled prop #1140

Merged
merged 10 commits into from
Aug 22, 2022
Merged

fix: add controlled prop #1140

merged 10 commits into from
Aug 22, 2022

Conversation

felix-ico
Copy link
Collaborator

@felix-ico felix-ico commented Jul 15, 2022

So using the same approach as in the dropdown fixes this react/reactivity issue, I guess this would need to be added to some other components as well to avoid the same issue in the future (at least in text-area).

@render
Copy link

render bot commented Jul 15, 2022

Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

Also, should we also document this new controlled prop in Storybook? I think yes, but with a notice saying it's "experimental"… maybe?

@@ -96,6 +96,8 @@ export class TextField {
/** (optional) Injected CSS styles */
@Prop() styles?: string;

/** (optional) Makes type `input` behave as a controlled component in React */
@Prop() controlled?: boolean = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking whether we should prefix this with "experimental", what do you think? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so i replaced (optional) with (experimental) but I'm not sure if that is what you meant or to change the prop name to something like experimental_controlled ? Wasn't sure because in other components like the dropdown there is also a controlled prop but without any experimental prefix

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, haha, I was thinking about experimental_controlled — the old Dropdown is going to be deprecated.

Would it work with the snake_case? 🤔

@@ -96,6 +96,8 @@ export class TextField {
/** (optional) Injected CSS styles */
@Prop() styles?: string;

/** (optional) Makes type `input` behave as a controlled component in React */
@Prop() controlled?: boolean = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, haha, I was thinking about experimental_controlled — the old Dropdown is going to be deprecated.

Would it work with the snake_case? 🤔

@acstll
Copy link
Collaborator

acstll commented Aug 11, 2022

I got confused with the many "conversations" I opened in the comments, haha, sorry 🙏

@felix-ico
Copy link
Collaborator Author

@acstll all CI jobs are passing now, snake_case doesn't work so i changed it to experimental-controlled.

I also added the prop to storybook, but there is no real documentation on why/how someone would use it. Should that be included in storybook as well? or maybe just in the readme?

Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

🙈

@felix-ico felix-ico merged commit fb828fb into main Aug 22, 2022
@felix-ico felix-ico deleted the fix/react-controlled-text-field branch August 22, 2022 15:18
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.

2 participants