feat(renderComponent): merged state and props for styles and accessibility #173

bmdalex · 2018-09-03T09:11:24Z

Component style function

This PR adds the ability to receive a unified (merged) object of props and state as argument to component's style functions, as well as to the accessibility behaviors.

More details in the dedicated issue: #161

TODO

API Proposal

Before

const buttonStyles: IComponentPartStylesInput = {
  root: ({
    props,
    variables
  }: { props: IButtonProps; variables: IButtonVars }): ICSSInJSStyle => {
    // props contains all props properties of the button
    return {}
  },
}

After

const buttonStyles: IComponentPartStylesInput = {
  root: ({
    props,
    variables
  }: { props: IButtonProps & IButtonState; variables: IButtonVars }): ICSSInJSStyle => {
    // props contains props and state properties merged, with props taking priority
    // in case of overlapping properties
    return {}
  },
}

codecov · 2018-09-03T09:11:28Z

Codecov Report

Merging #173 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #173   +/-   ##
=======================================
  Coverage   68.02%   68.02%           
=======================================
  Files         101      101           
  Lines        1370     1370           
  Branches      269      260    -9     
=======================================
  Hits          932      932           
  Misses        436      436           
  Partials        2        2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82f0c56...d958935. Read the comment docs.

kuzhelov · 2018-09-03T10:06:19Z

test/specs/commonTests/stylesFunction-test.tsx

+    })
+  })
+
+  it('simple merge of props and state if both are set but there are no overlapping properties', () => {


it would be better to describe result in the test name, like props and state properties are merged together and props properties take precedence over ones of state for the next one

I don't think I understand, let's discuss

discussed with @kuzhelov and amended

kuzhelov · 2018-09-03T10:07:03Z

src/lib/renderComponent.tsx

@@ -83,7 +73,11 @@ const renderComponent = <P extends {}>(

        // Resolve styles using resolved variables, merge results, allow props.styles to override
        const mergedStyles = mergeComponentStyles(componentStyles[displayName], props.styles)
-        const styleParam: ComponentStyleFunctionParam = { props, variables: resolvedVariables }
+        const appliedProps = { ...state, ...props }


one note - before we'll come up with a name for this 'merged' object, maybe we could use something that is a bit dumb but explicit and descriptive - like stateAndProps?

done, thanks

kuzhelov · 2018-09-03T10:09:57Z

src/lib/renderComponent.tsx

-    ...props,
-    ...state,
-  })
+  return callable(customAccessibility || defaultAccessibility || DefaultBehavior)(props)


accessibility functions end up with being provided with state only now - but previously state data was provided as well (this was a correct and expected behavior)

@jurokapsiar is this correct here?
@kuzhelov anything to change?

accessibility need a merge of props and state. props should override state. looking at line 83, I think this is correctly implemented now.

I would rather change the name of the argument, because it is a bit misleading - to indicate that 'state' is merged to 'props'

done, thanks guys

kuzhelov · 2018-09-03T10:56:55Z

types/theme.d.ts

@@ -77,7 +86,7 @@ export interface ICSSInJSStyle extends React.CSSProperties {
 }

 export interface ComponentStyleFunctionParam {
-  props: IProps
+  props: IState & IProps


kuzhelov · 2018-09-03T11:00:19Z

types/theme.d.ts

@@ -20,7 +21,15 @@ type ObjectOf<T> = { [key: string]: T }
 // Props
 // ========================================================

-export type IProps = ObjectOf<any>
+export type IProps = Extendable<{
+  variables?: ComponentVariablesInput


this won't work for all the components, unfortunately (Provider as an example) - lets keep to be generic for now

kuzhelov · 2018-09-03T11:02:13Z

src/lib/renderComponent.tsx

 export interface IRenderConfig {
  className?: string
  defaultProps?: { [key: string]: any }
  displayName: string
  handledProps: string[]
-  props: IRenderConfigProps
-  state: { [key: string]: any }
+  props: IProps


lets use the type that was used before - as IProps is not generic enough to suite all the components if styles and variables are introduced

…ility parameters

kuzhelov

👍

bmdalex requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners September 3, 2018 09:11

bmdalex added the 🚀 ready for review label Sep 3, 2018

bmdalex force-pushed the feat/styles-add-state-to-props branch from 2709529 to 780090c Compare September 3, 2018 09:14

kuzhelov reviewed Sep 3, 2018

View reviewed changes

feat(renderComponent): merged state and props for styles and accessib…

d958935

…ility parameters

bmdalex force-pushed the feat/styles-add-state-to-props branch from 780090c to d958935 Compare September 3, 2018 12:44

kuzhelov approved these changes Sep 3, 2018

View reviewed changes

kuzhelov added ready for merge and removed 🚀 ready for review labels Sep 3, 2018

kuzhelov merged commit ca435ed into master Sep 3, 2018

kuzhelov deleted the feat/styles-add-state-to-props branch September 3, 2018 13:03

kuzhelov mentioned this pull request Sep 7, 2018

[RFC] Provide merged props and state to Styles and Accessibility #161

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(renderComponent): merged state and props for styles and accessibility #173

feat(renderComponent): merged state and props for styles and accessibility #173

bmdalex commented Sep 3, 2018 •

edited by kuzhelov

codecov bot commented Sep 3, 2018 •

edited

kuzhelov Sep 3, 2018

bmdalex Sep 3, 2018

bmdalex Sep 3, 2018

kuzhelov Sep 3, 2018

kuzhelov Sep 3, 2018

bmdalex Sep 3, 2018

kuzhelov Sep 3, 2018

bmdalex Sep 3, 2018

jurokapsiar Sep 3, 2018

kuzhelov Sep 3, 2018

bmdalex Sep 3, 2018

kuzhelov Sep 3, 2018

kuzhelov Sep 3, 2018 •

edited

bmdalex Sep 3, 2018

kuzhelov Sep 3, 2018

bmdalex Sep 3, 2018

kuzhelov left a comment

feat(renderComponent): merged state and props for styles and accessibility #173

feat(renderComponent): merged state and props for styles and accessibility #173

Conversation

bmdalex commented Sep 3, 2018 • edited by kuzhelov

Component style function

TODO

API Proposal

Before

After

codecov bot commented Sep 3, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Sep 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov left a comment

Choose a reason for hiding this comment

bmdalex commented Sep 3, 2018 •

edited by kuzhelov

codecov bot commented Sep 3, 2018 •

edited

kuzhelov Sep 3, 2018 •

edited