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

Allow uncontrolled usage of form input components #172

Open
HendrikThePendric opened this issue Nov 11, 2019 · 9 comments
Open

Allow uncontrolled usage of form input components #172

HendrikThePendric opened this issue Nov 11, 2019 · 9 comments

Comments

@HendrikThePendric
Copy link
Contributor

As discussed in this public Slack thread, we should add support for uncontrolled usage to all of our form input components.

If any of the conditions below are problematic, we should discuss before proceeding:

  • Can be implemented in a consistent way for all form input components
  • Can be implemented without introducing any breaking changes
  • Can be implemented without adding too much complexity to the code-base
@Mohammer5
Copy link
Contributor

I haven't really bothered to try it, but I think we could remove the .isRequired from the value props of our input components and they'd already work uncontrolled as well.

The *Select components are a different story though, they'll have to check if there's a value prop and store the value themselves if there's no provided value (undefined = uncontrolled; null = no selected value? This will be a bit tricky I think)

@HendrikThePendric
Copy link
Contributor Author

I haven't really looked into it very closely yet either, but we both know the code-base quite well, so should be able to make a pretty educated guess.... I agree that for some of the atoms, all that is required is to remove .isRequired from the value prop. But at the same time I can think of a quite few components that will require some work:

  • The ToggleGroup (which powers Checkbox/Radio/Switch-Group) will need some internal state to keep the selected value.
  • TextArea will need some work to make the auto-grow work
  • Select (as you mention) I don't know, but since it's a custom input component it definitely won't work out of the box.
  • FileInput I worry about the most. I think that would need quite a lot of work done to it. We might even end up with two versions, because the behaviour of the uncontrolled version will diverge a lot from the controlled one: In the uncontrolled version the component itself needs to manage the FileListItems, but in the controlled implementation this is expected to be managed by the parent component (i.e. see here)

So, if we are to work on this, the Select and FileInput would probably be best places to start. If we can make these work as uncontrolled inputs, without having to jump through too many hoops, we can probably proceed. Otherwise we should discuss things further...

@Mohammer5
Copy link
Contributor

Yeah, the FileInput seems to be a weird one.. ToggleGroup seems do-able. Textarea is hard to judge for me right now

@ismay what do you think about the select components? Fairly simple and straight forward?

@HendrikThePendric
Copy link
Contributor Author

HendrikThePendric commented Nov 12, 2019

Textarea is hard to judge for me right now

That one seems pretty easy to fix. It currently does this:

    componentDidUpdate() {
        if (this.shouldDoAutoGrow()) {
            this.setHeight()
        }
    }

So the auto-grow logic is executed in the componentDidUpdate lifecycle hook. And this works because for a controlled component componentDidUpdate will be called whenever the value changes.

So if we want to support an uncontrolled version, we need to also have this logic in the handleChange method and execute it in case typeof props.value === 'undefined'. I suppose we'd also keep it in componentDidUpdate and execute it if typeof props.value !== 'undefined'...

@ghost
Copy link

ghost commented Nov 12, 2019

@ismay what do you think about the select components? Fairly simple and straight forward?

Well we'd have to introduce state somewhere. It's a bit hard to predict completely, but I guess we could introduce state in the SingleSelect and MultiSelect components, and if there's no value passed it would default to changing local state.

It might actually be a breaking change, as currently we default to a {} or [] when no value's passed, and it's still controlled.

We could use the following api:

SingleSelect:

  • selected={undefined} -> uncontrolled
  • selected={{}} -> controlled (or selected={null})
  • selected={{ value, label }} -> controlled

MultiSelect:

  • selected={undefined} -> uncontrolled
  • selected={[]} -> controlled
  • selected={[{ value, label }]} -> controlled

@varl
Copy link
Contributor

varl commented Nov 25, 2019

To support both controlled and uncontrolled workflows we would need to separate how to set values, examples given:

  • value => controlled, initialValue => uncontrolled
  • selected => controlled, initialSelected => uncontrolled

As for the FileInput component, React docs says it's always uncontrolled: https://reactjs.org/docs/uncontrolled-components.html#the-file-input-tag

Maybe we should only have one component where it always manages its own state? Not sure about this one.

@ghost
Copy link

ghost commented Nov 26, 2019

I do wonder if this point from @HendrikThePendric's top comment isn't applicable here:

Can be implemented without adding too much complexity to the code-base

I remember that material-ui allowed both controlled and uncontrolled components, but would throw an error if you switched from one to the other for an already mounted component. I'm not sure if that would be possible for our eventual implementation as well, but it does seem like we could be introducing more complexity for ourselves and the user in implementing this.

Maybe, if we still really want this, we could look at implementing this in an entirely separate repo/package. That should be very doable, and it would force a clean separation between what we have in ui-core, and the convenience wrappers for users who want them. Plus it'd be a subtle hint that the recommended usage is actually a controlled component.

@HendrikThePendric
Copy link
Contributor Author

I was also considering perhaps introducing a HOC that would have internal state and renders the controlled input. That way we would:

  • Avoid having too much complexity in the input components
  • Send a subtle signal that we would prefer people to use the controlled version

I haven't thought about the technical implementation much, so could be that it's not possible to build this in a generic way.... But in any case, I would really like a solution that avoids us from making our current inputs more complex.

@ghost ghost transferred this issue from dhis2/ui-core May 19, 2020
@HendrikThePendric
Copy link
Contributor Author

Adding this to the v6.0.0 milestone as this would be a breaking change and we are not introducing it in v5.0.0

@varl varl transferred this issue from dhis2/ui Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants