Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

feat: introduce counterparts for ui-core form components#108

Merged
HendrikThePendric merged 82 commits intomasterfrom
feat/ui-core-input-counterparts
Dec 12, 2019
Merged

feat: introduce counterparts for ui-core form components#108
HendrikThePendric merged 82 commits intomasterfrom
feat/ui-core-input-counterparts

Conversation

@HendrikThePendric
Copy link
Copy Markdown
Contributor

@HendrikThePendric HendrikThePendric commented Nov 6, 2019

READY FOR REVIEW

Work that has been done

  • Implemented al ui-core components
  • Improved the project structure
  • Moved away from class based components and started using hooks
  • Added e2e test for all components

Some specific points of attention:

  • FileInput is still a class component because I'd need an onRemove callback on the FileListItems which are being mapped over. I believe hooks have some trouble in these type of scenarios. But I'd prefer to hook this component up too if possible. (resolved)
  • In the SwitchGroup and CheckboxGroup stories there are some examples of using our components being used in tandem with format / parse props on the field to produce different types of form data. I think this demonstrates (one of) the biggest advantages of using our inputs as value of the component prop on the RFF Field component.
  • Because switching to hooks simplified things quite a bit for me, I wonder if perhaps the FieldAdapter should be implemented as a hook too.... (resolved)

Next steps

  1. Add validators: consolidate what we have already in various repos and implement, and add unit-tests to these
  2. Documentation: setup documentation publication pipeline, improve readme, add jsDoc comments everywhere
  3. Add widgets (at some point)

I have added this section firstly to inform about what I'm going to do once this is merged, but also to indicate that suggestions for improvements reg. the points above are regarded as out-of-scope (this PR already includes enough changes).

Implemented components

@HendrikThePendric HendrikThePendric self-assigned this Nov 6, 2019
Moved to the `./.storybook` folder
Switched to logging values directly, which is good for non-serializable things, such as File instances
- Stop using Toggle components to extract shared logic because it gets hard to reason about all these nested components
- Implement shared hooks and propTypes to replace Toggle components
- Update Checkbox/Switch(Group) to work with these shared hooks and proptypes
- Remove `useOnChange` prop from FieldAdapter component, because this way of working is also hard to reason about
- Refactor Checkbox and Switch to now implement onChange logic internally instead of delegating it to the FieldAdapter
@HendrikThePendric HendrikThePendric requested review from a user, Birkbjo, Mohammer5 and varl November 21, 2019 16:25
@HendrikThePendric HendrikThePendric marked this pull request as ready for review November 21, 2019 16:34
@HendrikThePendric
Copy link
Copy Markdown
Contributor Author

@ismay @varl I have finished discussing and implementing all review comments (mostly just in this branch, but two things have been deferred and I have created issues for that). So ready for re-re-re-review

@ghost
Copy link
Copy Markdown

ghost commented Dec 11, 2019

One thing we should probably double check: the required prop has been removed from Switch, Checkbox and Radio in ui-core@4. We were talking above about the usefullness of a required single checkbox, switch, etc. Maybe it's not even possible with ui-core@4? And if so, why did the integration tests not catch this?

@HendrikThePendric
Copy link
Copy Markdown
Contributor Author

One thing we should probably double check: the required prop has been removed from Switch, Checkbox and Radio in ui-core@4. We were talking above about the usefullness of a required single checkbox, switch, etc. Maybe it's not even possible with ui-core@4? And if so, why did the integration tests not catch this?

I can understand the confusion when reading the release notes. That's because for the inputs, some of the functionality was moved from ComponentName to ComponentNameField. This goes for the Checkbox (see storybook) and Switch (see storybook) too. In ui-forms I am using CheckboxField and SwitchField, so required is working fine. A single Radio is not a usable form input, so we can disregard that.

So, I have double checked this, and it is working as expected.

@ghost ghost self-requested a review December 11, 2019 13:26
@ghost
Copy link
Copy Markdown

ghost commented Dec 11, 2019

One thing we should probably double check: the required prop has been removed from Switch, Checkbox and Radio in ui-core@4. We were talking above about the usefullness of a required single checkbox, switch, etc. Maybe it's not even possible with ui-core@4? And if so, why did the integration tests not catch this?

I can understand the confusion when reading the release notes. That's because for the inputs, some of the functionality was moved from ComponentName to ComponentNameField. This goes for the Checkbox (see storybook) and Switch (see storybook) too. In ui-forms I am using CheckboxField and SwitchField, so required is working fine. A single Radio is not a usable form input, so we can disregard that.

So, I have double checked this, and it is working as expected.

Ah I see, that makes sense 👍

Comment thread src/components/SingleSelect.js
Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Left a couple of remarks. We could defer those until later, as we've done with other topics that were out of scope for these changes. Seems fine to merge to me 👍

@HendrikThePendric HendrikThePendric merged commit 9dbeb29 into master Dec 12, 2019
@HendrikThePendric HendrikThePendric deleted the feat/ui-core-input-counterparts branch December 12, 2019 12:51
dhis2-bot added a commit that referenced this pull request Feb 13, 2020
# 1.0.0 (2020-02-13)

### Bug Fixes

* **deps:** move @dhis2/props-types back to dependencies ([#156](#156)) ([9354cbc](9354cbc))
* **deps:** set dependencies provided by app-platform to peerDeps ([#152](#152)) ([db38005](db38005))
* force the optimize for speed flag for styled-jsx based on env ([5626711](5626711))
* force the optimize for speed flag for styled-jsx to true ([71183c1](71183c1))
* **conditional:** use render instead of onChange to have access to form ([615488d](615488d))
* **file:** show name of selected file ([d622b34](d622b34))

### Features

* add composeValidators & required and email validator functions ([9d6ec6a](9d6ec6a))
* add confitional HOC ([e677b7b](e677b7b))
* add File component ([308335f](308335f))
* add Form component & mutators ([2b0ba72](2b0ba72))
* add Radio and RadioGroup & update stories ([b93ce20](b93ce20))
* add required validator ([a7a5c37](a7a5c37))
* apply conditional HOC to File ([8559f28](8559f28))
* introduce counterparts for ui-core form components ([#108](#108)) ([9dbeb29](9dbeb29)), closes [#133](#133) [#134](#134) [#136](#136) [#146](#146)
* synchronous validators ([#164](#164)) ([75cd2ea](75cd2ea))
* **conditional:** conditional as a regular component ([#16](#16)) ([8f6eae7](8f6eae7))
@dhis2-bot
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants