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

refactor: kill the filled option for selectfield and inputfield #427

Merged
merged 11 commits into from
Sep 17, 2019
Merged

Conversation

varl
Copy link
Contributor

@varl varl commented Sep 16, 2019

@cooper-joe This removes the Filled options for InputField and SelectField, leaving one master style. ⚔️

@netlify
Copy link

netlify bot commented Sep 16, 2019

Deploy preview for dhis2-ui-core ready!

Built with commit 208e04c

https://deploy-preview-427--dhis2-ui-core.netlify.com

Copy link
Contributor

@Mohammer5 Mohammer5 left a comment

Choose a reason for hiding this comment

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

The InputField includes the Help text, which makes sense I assume (I think this touches on @HendrikThePendric is doing in ui-forms, maybe he wants to say sth specific to this)

The SelectField does not have a Help text, I think we should make that consisten

@varl
Copy link
Contributor Author

varl commented Sep 16, 2019

The InputField includes the Help text, which makes sense I assume (I think this touches on @HendrikThePendric is doing in ui-forms, maybe he wants to say sth specific to this)

The SelectField does not have a Help text, I think we should make that consisten

Yeah, the SelectField did not get the same treatment on Friday, though it has now in e6f0715.

@varl varl marked this pull request as ready for review September 16, 2019 15:37
@varl varl requested a review from a team as a code owner September 16, 2019 15:37
src/Help.js Show resolved Hide resolved
@varl varl requested a review from Mohammer5 September 16, 2019 15:40
@varl
Copy link
Contributor Author

varl commented Sep 16, 2019

@cooper-joe There might be some styling issues that we should clean up, are there any obvious ones in InputField and/or SelectField you see? Like should the Label have a dense option?

@cooper-joe
Copy link
Member

@varl These are the minor style changes I can see need to be made. I had a play around but wasn't sure whether they'd be applied to the input file or the inputField/selectField file, so I'll let you take care of that!

These changes apply to all input fields and select fields, in all states, unless otherwise noted:

  • border-radius: 3px replaces border-radius: 4px
  • Default border color changes to grey500 from theme.default (this should not be themeable).
  • disabled fields get background-color: grey100
  • All fields get box-shadow: inset 0 0 0 1px rgba(102,113,123,0.15), inset 0 1px 2px 0 rgba(102,113,123,0.10);
  • padding on input changes to padding: 13px 0 11px 11px;

That's everything I see, thanks!

@cooper-joe cooper-joe self-requested a review September 17, 2019 07:43
Copy link
Contributor

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

As @Mohammer5 noted: the InputField now includes a Help field. And it seems we are heading into that direction for all input fields e6f0715. There might be reasons for heading that way, but I personally think it would be better not to do so.

By keeping the Help out of the input fields in ui-core we keep our components "feel" much more lightweight and composable. Suppose that in an app you need a CheckboxGroups input field with the following specs:

  • There are multiple groups
  • Each group has a header and a list of checkboxes
  • All these checkboxes are just there to set values for one model property
  • There is only one error/help text for all these groups

In our current setup (i.e. the Checkbox does not include a Help), composing the input field above feels really natural, because the Checkboxes are only the actual input (incl. label). If we were to move to a situation where input fields also include a help and an error text, making the above input field would still be possible, but the developer would feel like he/she is slightly "misusing" our component. The Checkbox has an API to control the error text, but this is completely ignored, and instead we are adding some custom logic to display a separate Help text.

The way I envisaged things to work:

  • Fully functional form fields go into ui-forms
  • Building blocks to make custom form fields go into ui-core

Whether or not we stick to that distinction has implications on ui-core and ui-forms. We have a meeting about ui-forms this afternoon. I'll make sure to put this topic onto the agenda so we can form a consensus about it.

src/Help.js Show resolved Hide resolved
@varl
Copy link
Contributor Author

varl commented Sep 17, 2019

Awesome @cooper-joe. I've made the mods and thanks for the concrete list.

@varl
Copy link
Contributor Author

varl commented Sep 17, 2019

By keeping the Help out of the input fields in ui-core we keep our components "feel" much more lightweight and composable.

@HendrikThePendric The InputField molecule is composed from Field, Input, Help, and Label.

All of those building blocks are also exposed through ui-core, so you are free to use the same atoms to create different molecules somewhere else (e.g. ui-forms), but a fully formed InputField molecule should be exposed through ui-core as well for simpler use cases.

Copy link
Member

@cooper-joe cooper-joe left a comment

Choose a reason for hiding this comment

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

Thanks for updating those styles, LGTM!

Copy link
Contributor

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

The InputField molecule is composed from Field, Input, Help, and Label.

All of those building blocks are also exposed through ui-core, so you are free to use the same atoms to create different molecules somewhere else (e.g. ui-forms), but a fully formed InputField molecule should be exposed through ui-core as well for simpler use cases.

OK. This actually makes a lot of sense as a general pattern. And this is also in line with something @Mohammer5 mentioned on Slack yesterday....
So the naming convention will be that components ending with Field are the fully formed ones?

This will trigger the migration of some ui-forms components to ui-core. That's is probably a good development, because I am currently exporting two "flavours" of a component from there and that's not ideal... We might also want to reconsider some of the helper components we introduced recently, and perhaps the file-input would be implemented slightly differently too. But that's all unrelated to this PR.

So approving this now.

@varl
Copy link
Contributor Author

varl commented Sep 17, 2019

So the naming convention will be that components ending with Field are the fully formed ones?

Yes.

This will trigger the migration of some ui-forms components to ui-core.

Right. 👍

One more thing, I'm moving all the styles in the {Input,Select}Field components into the lower level building blocks {Input,Select} to ensure that the -Field component is 100% composable, see 1eeaa6a.

@varl varl merged commit 561a0a6 into next Sep 17, 2019
@varl varl deleted the kill-filled branch September 17, 2019 11:54
varl added a commit that referenced this pull request Nov 6, 2019
* refactor: remove filled fields

* refactor: move molecules to *Field

* refactor(select,input): reorganising components

* refactor(inputfield): add proper input molecule

* refactor(selectfield): give selects the same treatment as input

* docs: link to demo site

* docs: update link ref

* docs: add no label story for selectfield

* refactor: update styles for fields

* refactor(input,inputfield): move styles to lower level component

* refactor(select,selectfield,label): update components for isolation
varl added a commit that referenced this pull request Nov 6, 2019
* refactor: remove filled fields

* refactor: move molecules to *Field

* refactor(select,input): reorganising components

* refactor(inputfield): add proper input molecule

* refactor(selectfield): give selects the same treatment as input

* docs: link to demo site

* docs: update link ref

* docs: add no label story for selectfield

* refactor: update styles for fields

* refactor(input,inputfield): move styles to lower level component

* refactor(select,selectfield,label): update components for isolation
varl added a commit that referenced this pull request Nov 7, 2019
BREAKING CHANGE: Remove the Filled field style for SelectField and
InputField.
varl added a commit that referenced this pull request Nov 7, 2019
BREAKING CHANGE: Remove the Filled field style for SelectField and
InputField.
varl added a commit that referenced this pull request Nov 7, 2019
BREAKING CHANGE: Remove the Filled field style for SelectField and
InputField.
varl added a commit that referenced this pull request Nov 8, 2019
BREAKING CHANGE: Remove the Filled field style for SelectField and
InputField.
varl added a commit that referenced this pull request Nov 20, 2019
BREAKING CHANGE: Remove the Filled field style for SelectField and
InputField.
varl added a commit that referenced this pull request Nov 25, 2019
BREAKING CHANGE: Remove the Filled field style for SelectField and
InputField.
varl added a commit that referenced this pull request Nov 25, 2019
BREAKING CHANGE: Remove the Filled field style for SelectField and
InputField.
dhis2-bot added a commit that referenced this pull request Nov 25, 2019
# [4.0.0](dhis2/ui@v3.12.0...v4.0.0) (2019-11-25)

### Bug Fixes

* **alert-stack:** revert changes so it just uses a fixed z-index ([63b6cff](dhis2/ui@63b6cff))
* **checkbox:** align focus/blur with change and remove disabled check ([7527002](dhis2/ui@7527002))
* **checkbox,radio:** value should be optional ([#419](dhis2/ui#419)) ([05306a2](dhis2/ui@05306a2))
* **chip remove icon:** fix position cross browser ([40d86a0](dhis2/ui@40d86a0))
* **css-variables:** improve robustness and developer warnings ([#449](dhis2/ui#449)) ([352a4d1](dhis2/ui@352a4d1))
* **file-field:** consistently use FileField instead of File + fix typo ([d51b119](dhis2/ui@d51b119))
* **file-input:** adjust component names - FormControl to Field ([d31e861](dhis2/ui@d31e861))
* **file-input:** correct line height and add className prop ([c6b79d5](dhis2/ui@c6b79d5))
* **file-input:** fix Edge text overflow behavior ([2f4a1cf](dhis2/ui@2f4a1cf))
* **file-input:** implement PlaceHolder as child of FileList ([4d70143](dhis2/ui@4d70143))
* **file-input:** implement spacing between text and link via ::after el ([f4ae821](dhis2/ui@f4ae821))
* **file-input:** improve children prop-type to allow multiple files ([e7785fd](dhis2/ui@e7785fd))
* **file-input:** introduce FileList to solve spacing issues ([5faae50](dhis2/ui@5faae50))
* **file-input:** tweak CSS to fix incorrect total height ([79d8df0](dhis2/ui@79d8df0))
* **input:** align handleFocus/handleBlur with handleChange ([848d5fe](dhis2/ui@848d5fe))
* **input:** allow types specified in design-system ([29127f3](dhis2/ui@29127f3))
* **input:** correct prop-type for type ([066e468](dhis2/ui@066e468))
* **input:** remove duplicate story and improve "No label" ([#480](dhis2/ui#480)) ([0a40894](dhis2/ui@0a40894))
* **input-field:** add required `onChange` prop to all stories ([d0060bc](dhis2/ui@d0060bc))
* **input-field:** adjust focus props and label story ([#447](dhis2/ui#447)) ([43df32e](dhis2/ui@43df32e))
* **layer:** children must be a function and required ([ec75085](dhis2/ui@ec75085))
* **layer context:** simplify and correct calculations ([4a14e3f](dhis2/ui@4a14e3f))
* **layer-context:** add support for layers upon layers ([677e585](dhis2/ui@677e585))
* **prop-types:** adjust statusPropType and use custom one for AlertBar ([85d40ba](dhis2/ui@85d40ba))
* **radio:** align handleFocus/handleBlur with handleChange ([d9b6305](dhis2/ui@d9b6305))
* **select:** add missing prop types ([fc4e499](dhis2/ui@fc4e499))
* **select:** add missing tab index default ([85904e3](dhis2/ui@85904e3))
* **select:** align onfocus and onblur with other cbs ([a607f31](dhis2/ui@a607f31))
* **select:** align select with callback style changes ([456d116](dhis2/ui@456d116))
* **select:** fix overlap on specifying input width for flex children ([54a15aa](dhis2/ui@54a15aa))
* **select:** fix positioning issues ([6bd8a2b](dhis2/ui@6bd8a2b))
* **select:** fix select closing on chip removal ([b51d101](dhis2/ui@b51d101))
* **text-area:** only set height when value changes ([9ec7437](dhis2/ui@9ec7437))
* **toggle-group:** remove `.isRequired` from `value` prop ([abb790c](dhis2/ui@abb790c))
* adjust menu position after option selection ([a5b9385](dhis2/ui@a5b9385))
* correct handler usage for filterable menu ([7f53a63](dhis2/ui@7f53a63))
* export Label in index.js ([85187b9](dhis2/ui@85187b9))
* introduce layer-context to deal with overlapping elements + story ([6632093](dhis2/ui@6632093))
* use layer-context in alert-stack and drop-menu ([2caece4](dhis2/ui@2caece4))
* **select:** make filtering case insensitive ([6d997a8](dhis2/ui@6d997a8))
* **split-button:** remove unused styles ([134fd16](dhis2/ui@134fd16))
* **switch:** align handleFocus/handleBlur with handleChange ([92550c8](dhis2/ui@92550c8))
* **tab-bar:** enforce that children are instances of the Tab component ([d583188](dhis2/ui@d583188))
* **text-area:** align handleFocus/handleBlur with handleChange ([2f3ad48](dhis2/ui@2f3ad48))
* make name prop optional instead of required ([ab8f6a5](dhis2/ui@ab8f6a5))
* **switch:** change input type to checkbox because switch is not valid ([245f8bf](dhis2/ui@245f8bf))
* **toggles:** remove position relative from toggles ([a3f5e2e](dhis2/ui@a3f5e2e))

### chore

* upgrade react to support hooks ([35672e0](dhis2/ui@35672e0))
* **help:** remove `indent` props and examples from stories ([#471](dhis2/ui#471)) ([12efecb](dhis2/ui@12efecb))

### Code Refactoring

* **checkbox:** align 'on' function handlers ([53c700d](dhis2/ui@53c700d))
* **fileinput:** align 'on' function handlers ([8829eec](dhis2/ui@8829eec))
* **formcontrol:** rename to field ([#402](dhis2/ui#402)) ([b9205e1](dhis2/ui@b9205e1))
* **input:** align 'on' function handlers ([2ab28b7](dhis2/ui@2ab28b7))
* **input,textarea:** remove option to pass in number for width ([77105f2](dhis2/ui@77105f2))
* **modal:** align modal with component structure ([b276a31](dhis2/ui@b276a31))
* **modal:** remove open prop ([9da92f1](dhis2/ui@9da92f1))
* **modal:** remove unnecessary aside element wrapper ([5e39456](dhis2/ui@5e39456))
* **radio:** align 'on' function handlers ([611a394](dhis2/ui@611a394))
* **select:** update component to new design ([e810a78](dhis2/ui@e810a78))
* **splitbutton:** separate component from buttonstrip ([#396](dhis2/ui#396)) ([f24cd11](dhis2/ui@f24cd11))
* **switch:** align 'on' function handlers ([5011d6d](dhis2/ui@5011d6d))
* **tabbar:** align component structure ([e3313f3](dhis2/ui@e3313f3))
* **textarea:** align 'on' function handlers ([44ef1bf](dhis2/ui@44ef1bf))
* implement the Radio component consistently with other inputs ([8fed62f](dhis2/ui@8fed62f))
* kill the filled option for selectfield and inputfield ([#427](dhis2/ui#427)) ([e0eabdb](dhis2/ui@e0eabdb))
* toggle input components ([b51b71c](dhis2/ui@b51b71c))
* **select,input:** drop -field suffix on components ([#389](dhis2/ui#389)) ([eb104f5](dhis2/ui@eb104f5))
* **toggle-group:** split into composable components ([#388](dhis2/ui#388)) ([e4beb91](dhis2/ui@e4beb91))

### Documentation

* introduce user docs ([#397](dhis2/ui#397)) ([c3bc557](dhis2/ui@c3bc557))

### Features

* **field:** add className prop ([7565d59](dhis2/ui@7565d59))
* **file-field:** introduce FileField component ([0423f62](dhis2/ui@0423f62))
* **file-input:** add disabled prop and button sizes ([066538f](dhis2/ui@066538f))
* **input atom:** add ellipsis to overflowing text ([56ce804](dhis2/ui@56ce804))
* **label:** add className prop ([8526560](dhis2/ui@8526560))
* **modal:** add className prop ([6c231d6](dhis2/ui@6c231d6))
* **node:** add className prop to node ([1468e35](dhis2/ui@1468e35))
* **screencover:** add transparent prop ([90a5881](dhis2/ui@90a5881))
* **select:** add input max height prop ([75555b3](dhis2/ui@75555b3))
* **select:** add inputWidth property to control width ([9eefe22](dhis2/ui@9eefe22))
* **status-icon:** add `info` and `defaultTo` props ([8144311](dhis2/ui@8144311))
* **tab components:** add className props ([480f538](dhis2/ui@480f538))
* **textarea:** introduce TextArea and TextAreaField with docs and stories ([#456](dhis2/ui#456)) ([0e73d7a](dhis2/ui@0e73d7a))
* **theme:** expose spacers, elevations and layers constants ([76f34ba](dhis2/ui@76f34ba))

### BREAKING CHANGES

* **modal:** Removed the aside element wrapper
* **modal:** Removing the "open" prop from the Modal.
* **input,textarea:** Input and TextArea now requires the input width
override to be passed in as a string.
* Now requires React >= 16.8.
* **fileinput:** `onChange` is called with two parameters:
First parameter an object with properties for `checked`, `name`, and
`value`.
The second is the React `event` object.
* **textarea:** `onChange` is called with two parameters:
First parameter an object with properties for `checked`, `name`, and
`value`.
The second is the React `event` object.
* **input:** `onChange` is called with two parameters:
First parameter an object with properties for `checked`, `name`, and
`value`.
The second is the React `event` object.
* **checkbox:** `onChange` is called with two parameters:
First parameter an object with properties for `checked`, `name`, and
`value`.
The second is the React `event` object.
* **radio:** `onChange` is called with two parameters:
First parameter an object with properties for `checked`, `name`, and
`value`.
The second is the React `event` object.
* **switch:** `onChange` is called with two parameters:
First parameter an object with properties for `checked`, `name`, and
`value`.
The second is the React `event` object.
* **tabbar:** This replaces the need to nest a TabBar in a ScrollBar,
and instead allows `TabBar.propTypes.scrollable` to toggle if a TabBar
is scrollable.
* **tabbar:** This deprecates external use of ScrollBar.
* **modal:** This deprecated use of Modal.Actions. Instead, use
ModalActions.
* **modal:** This deprecated use of Modal.Content. Instead, use
ModalContent.
* **modal:** This deprecated use of Modal.Title. Instead, use
ModalTitle.
* **tab-bar:** TabBar will now only accept Tab instances
* **select:** Select deprecated in favor of the SingleSelect, refer
to the docs for the API.
* **select:** SelectField deprecated in favor of SingleSelectField,
see the docs for the API.
* **select:** The new Select components do not accept a standard HTML `option`
element, instead they require use of SingleSelectOption, MultiSelectOption,
or a component with a matching API.
* **select:** The Select's `onChange` no longer returns a standard `Event.target`,
instead returns the selected Object with properties: `label` and
`string`, or an array of those for the MultiSelects
* **select:** The Selects all require a selected prop, instead of a value.
Where selected is an object with a label and a value string, or an array of
those for the MultiSelect.
* **select:** Since the Selects do not use a native input element, the name
prop has been dropped from the Select components.
* The `icon` and `required` props have been removed from
the Checkbox and Switch. For Switch, the `label` prop has become
required.
* RadioGroup has a new implementation and drops the
following props: label, inline, options, and helpText.
* The `icon` and `required` props have been removed from the Radio component and the `value` and `label` props have become required.
* **help:** Removes indent prop from Help. Use className to
override if necessary.
* Remove the Filled field style for SelectField and
InputField.
* MenuItem value prop is more strict, requires String
instead of any.
* **formcontrol:** Renames FormControl to Field.
* **splitbutton:** remove the `compact` prop from buttonstrip
* **toggle-group:** Renames ToggleGroup to FieldSet to match intended
usage.
* **select,input:** **InputField** renamed to **Input**

**SelectField** renamed to **Select**

* chore: trigger netlify
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants