fix(css-variables): improve robustness and developer warnings #449

HendrikThePendric · 2019-09-23T14:17:43Z

The thinking behind this is:

Because we now loop through the object keys of allowedKeys we prevent nasty errors
Since the previous change cause things to fail silently (i.e. developer adds prop shape which isn't in the allowedKeys so it's simply ignored), I added some props-validation that would let the developer know he has added a not-allowed prop.

Currently this PR has draft status because it is dependent on dhis2/prop-types#46. Once that's been merged, we still need to update the version of @dhis2/prop-types here....

netlify · 2019-09-23T14:19:50Z

Deploy preview for dhis2-ui-core ready!

Built with commit b439c09

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

src/CssVariables.js

varl · 2019-09-23T14:44:28Z

Why are we being clever about this at all?

We can pick the exact props we want from both the theme and the props, and if someone wants to pass in foobar we just ignore it.

varl · 2019-09-23T14:49:29Z

OK I found the discussion that sparked this here: #446 (comment)

HendrikThePendric · 2019-09-23T14:54:59Z

OK I found the discussion that sparked this

Yeah you beat me to it. Damn. I would personally be fine just letting it fail silently. If we chose to do so, we could also consider just closing: dhis2/prop-types#46

Just to clarify:

I would be fine skipping forbidUnknowProps part
But I would like to keep working with a notion of allowedProps, because I didn't like how that was implemented before

varl · 2019-09-23T15:13:15Z

If we assume that:

We close feat: introduce forbidUnknowProps prop-types#46
We skip the forbidUnknownProps

Then:

But I would like to keep working with a notion of allowedProps, because I didn't like how that was implemented before

How would you change this PR to align it with that remark?

HendrikThePendric · 2019-09-23T15:21:09Z

How would you change this PR to align it with that remark?

Like this: 38af6c5

varl · 2019-09-23T15:22:43Z

.eslintrc.js

+        propWrapperFunctions: [
+            'whitelistProps',
+            { property: 'whitelistProps', object: 'propTypes' },
+        ],


Why is this necessary and will it be required everywhere we want to use the function?

Based on the last commit this should also be removed.

varl · 2019-09-23T15:23:32Z

How would you change this PR to align it with that remark?

Like this: 38af6c5

All right.

varl · 2019-09-23T15:23:54Z

Fat fingers and touchpad.. Re-opened.

ismay · 2019-09-23T15:25:04Z

Since the prop-types are a bit more involved than just using propTypes.exact (it seems that's a bug), why not just keep CssVariables as it is currently?

It's simple and it'll fail if you use it wrong. Seems fine to me.

varl · 2019-09-23T15:30:59Z

Since the prop-types are a bit more involved than just using propTypes.exact (it seems that's a bug), why not just keep CssVariables as it is currently?

It's simple and it'll fail if you use it wrong. Seems fine to me.

If we change it according to @HendrikThePendric's commit, then we do not need to ignore the ESLint rule no-unused-props.

ismay · 2019-09-23T15:35:02Z

Since the prop-types are a bit more involved than just using propTypes.exact (it seems that's a bug), why not just keep CssVariables as it is currently?
It's simple and it'll fail if you use it wrong. Seems fine to me.

If we change it according to @HendrikThePendric's commit, then we do not need to ignore the ESLint rule no-unused-props.

That's true. I'll defer to what you and @HendrikThePendric prefer.

varl · 2019-09-23T15:44:00Z

I agree with the allowedProps change just to be able lint it properly without ignores.

HendrikThePendric · 2019-09-24T07:15:57Z

OK, just a short summary:

We're keeping the change in CssVariables that introduced allowedProps, because it removes the need for ignoring the lint rule and prevent the function from throwing errors on non-existent theme variables.
We're dropping the prop-types validation (whitelistProps) part, because the solution is a bit too involved for such a small problem.
So, that also means feat: introduce forbidUnknowProps prop-types#46 is now closed

Mohammer5

LGTM

* fix(css-variables): improve robustness and developer warnings * chore(eslint): remove unneeded env setting * chore(css-variables): fix typo * fix(css-variables): adjusted prop validator name * fix(css-variables): stop using whitelistProps because it's overkill * chore(eslint): remove redundant `propWrapperFunctions` config

# [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

HendrikThePendric added 2 commits September 23, 2019 16:10

fix(css-variables): improve robustness and developer warnings

2de7790

chore(eslint): remove unneeded env setting

3b60782

HendrikThePendric self-assigned this Sep 23, 2019

HendrikThePendric requested a review from ismay September 23, 2019 14:17

varl reviewed Sep 23, 2019

View reviewed changes

src/CssVariables.js Outdated Show resolved Hide resolved

chore(css-variables): fix typo

eb1e865

HendrikThePendric added 2 commits September 23, 2019 17:18

fix(css-variables): adjusted prop validator name

00924ba

fix(css-variables): stop using whitelistProps because it's overkill

38af6c5

varl reviewed Sep 23, 2019

View reviewed changes

varl closed this Sep 23, 2019

varl reopened this Sep 23, 2019

chore(eslint): remove redundant propWrapperFunctions config

b439c09

HendrikThePendric marked this pull request as ready for review September 24, 2019 07:07

HendrikThePendric requested a review from a team as a code owner September 24, 2019 07:07

HendrikThePendric mentioned this pull request Sep 24, 2019

feat: introduce forbidUnknowProps dhis2/prop-types#46

Closed

Mohammer5 approved these changes Sep 24, 2019

View reviewed changes

varl approved these changes Sep 24, 2019

View reviewed changes

HendrikThePendric merged commit 4c8ab1a into next Sep 24, 2019

HendrikThePendric deleted the feat/css-variable-prop-validation branch September 24, 2019 07:46

varl mentioned this pull request Sep 24, 2019

chore(release): cut version 4.0.0 #430

Merged

varl added the next label Sep 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(css-variables): improve robustness and developer warnings #449

fix(css-variables): improve robustness and developer warnings #449

HendrikThePendric commented Sep 23, 2019

netlify bot commented Sep 23, 2019 •

edited

Loading

varl commented Sep 23, 2019

varl commented Sep 23, 2019

HendrikThePendric commented Sep 23, 2019 •

edited

Loading

varl commented Sep 23, 2019

HendrikThePendric commented Sep 23, 2019 •

edited

Loading

varl Sep 23, 2019

varl Sep 23, 2019

varl commented Sep 23, 2019

varl commented Sep 23, 2019

ismay commented Sep 23, 2019

varl commented Sep 23, 2019 •

edited

Loading

ismay commented Sep 23, 2019

varl commented Sep 23, 2019

HendrikThePendric commented Sep 24, 2019

Mohammer5 left a comment

fix(css-variables): improve robustness and developer warnings #449

fix(css-variables): improve robustness and developer warnings #449

Conversation

HendrikThePendric commented Sep 23, 2019

netlify bot commented Sep 23, 2019 • edited Loading

varl commented Sep 23, 2019

varl commented Sep 23, 2019

HendrikThePendric commented Sep 23, 2019 • edited Loading

varl commented Sep 23, 2019

HendrikThePendric commented Sep 23, 2019 • edited Loading

varl Sep 23, 2019

Choose a reason for hiding this comment

varl Sep 23, 2019

Choose a reason for hiding this comment

varl commented Sep 23, 2019

varl commented Sep 23, 2019

ismay commented Sep 23, 2019

varl commented Sep 23, 2019 • edited Loading

ismay commented Sep 23, 2019

varl commented Sep 23, 2019

HendrikThePendric commented Sep 24, 2019

Mohammer5 left a comment

Choose a reason for hiding this comment

netlify bot commented Sep 23, 2019 •

edited

Loading

HendrikThePendric commented Sep 23, 2019 •

edited

Loading

HendrikThePendric commented Sep 23, 2019 •

edited

Loading

varl commented Sep 23, 2019 •

edited

Loading