feat(Accordion): add props interface #33

bmdalex · 2018-07-31T17:42:19Z

Component Interfaces

This PR introduces a fix for UIComponent props to make them work with React attributes

codecov · 2018-07-31T17:42:24Z

Codecov Report

Merging #33 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master      #33   +/-   ##
=======================================
  Coverage   88.37%   88.37%           
=======================================
  Files          47       47           
  Lines         774      774           
  Branches      101      110    +9     
=======================================
  Hits          684      684           
  Misses         87       87           
  Partials        3        3

Impacted Files	Coverage Δ
src/components/Accordion/Accordion.tsx	`66% <100%> (ø)`	⬆️

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 1da274c...aa26eb0. Read the comment docs.

bmdalex · 2018-07-31T17:43:39Z

@mnajdova will take over, it's probably good to go as is

kuzhelov

thank you 👍

kuzhelov · 2018-08-01T12:49:34Z

src/components/Avatar/avatarRules.ts

@@ -1,5 +1,5 @@
 import { pxToRem } from '../../lib'
-import { PositionProperty } from '../../../node_modules/csstype'
+import { PositionProperty } from 'csstype'


+1, thanks for fixing this

kuzhelov · 2018-08-01T14:32:44Z

src/components/Label/Label.tsx

  }

-  static handledProps = ['as', 'children', 'circular', 'className', 'content']
+  static handledProps = ['as', 'children', 'circular', 'className', 'content', 'variables']


kuzhelov · 2018-08-01T15:25:50Z

src/lib/UIComponent.tsx

-class UIComponent<P, S> extends React.Component<P, S> {
+export interface UIComponentProps extends HTMLAttributes<HTMLElement> {
+  as?: string | Function
+  variables?: (siteVariables: object) => object


actually, we should make it clear whether plain object could be accepted as value for variables prop - there are quite a few places where this approach is used now

my vote would be to support both options, due to otherwise we could introduce unnecessary additional complexity for the client of this code in certain cases (where it is about just providing some predefined variables independent of theming aspects)

I will address this, agreed.

This will go away with #16, which adds typings for all theming concerns:

https://github.com/stardust-ui/react/blob/418e94761913b8bfc1035982d115693a406d0c12/types/theme.d.ts

levithomason · 2018-08-02T02:23:24Z

src/lib/UIComponent.tsx

 import renderComponent, { IRenderResultConfig } from './renderComponent'

-class UIComponent<P, S> extends React.Component<P, S> {
+export interface UIComponentProps extends HTMLAttributes<HTMLElement> {


This will flood the user's intellisense with a lot of HTML prop noise. That will make it difficult to know which props are for the component specifically.

There have been some community proposals and discussions in SUIR for how to handle typings like this. The current resolution is to include only typings for the props our components specifically handle. See Semantic-Org/Semantic-UI-React#1072.

Beyond this, there is also some desire to have strict typings that do not allow [key: string]: any. The ideas have not yet been merged, but you can see the need and proposal here: Semantic-Org/Semantic-UI-React#2836

levithomason · 2018-08-02T02:25:25Z

Can we get more information on the issue at hand and how this solves it? I'm not sure this should be merged.

mnajdova · 2018-08-02T08:40:15Z

There were some interfaces added for some of the Components's props, that started complaining when using some props that were not included in the interface (like the variables prop). This is the reason why we added this logic. If we decide not to merge this, then I will create a PR that will delete the usage of the props interfaces for now, and once we have a clear idea how to proceed this, I will return to this. Is this ok? @kuzhelov @levithomason

kuzhelov · 2018-08-02T10:49:52Z

I agree with making a step back with the types support, and afterwards make an incremental changes that will steadily introduce this back (starting from the general properties that are library-wide, and moving to component-specific interfaces). While doing that, we should keep an eye on preventing IntelliSense flood, as @levithomason has noticed - as well as ensuring that this won't decrease developer's productivity with false compiler warnings and complains.

In general, support @mnajdova proposal on this.

levithomason · 2018-08-06T17:54:05Z

As a min bar, I think we should include typings for the current props. I don't see a downside to this as it offers intellisense. To avoid the issue of not allowing other props in the typings, we can just add [key: string]: any for now. This will at least keep track of which props our components use.

mnajdova · 2018-08-08T14:06:15Z

@levithomason that's actually a good point, but I would rather wait for the typescript build issues to be resolved first. What do you think?

levithomason · 2018-08-20T18:55:29Z

I have captured the thoughts here in #117. Let's add typings one component per PR. I'm sure there will be plenty of fixes / discussions on each as we go.

levithomason · 2018-08-20T19:02:21Z

@Bugaa92 I have scoped this PR to "Accordion" since it is first on the #117 list and I'd like to set precedent for linking PRs to each component there.

Would you like to update it to fix Accordion's props per our conclusions?

# Conflicts: # src/components/Avatar/avatarRules.ts # src/components/Button/Button.tsx # src/components/Icon/Icon.tsx # src/components/Label/Label.tsx # src/components/Text/Text.tsx # src/lib/UIComponent.tsx

mnajdova · 2018-08-23T12:31:39Z

Guys, I added initial proposal for the props interface for the Accordion in this PR. Please share your thoughts, and let's use this PR for discussing all aspects of the props interface definitions, so that after this is merged, we can apply the same on the other components as well. cc @Bugaa92 @kuzhelov @smykhailov @miroslavstastny @levithomason

jurokapsiar · 2018-08-23T13:10:43Z

src/components/Accordion/Accordion.tsx

+export interface IAccordionProps extends IAccordionPropsStrict {
+  [key: string]: any
+}
+


you can try to define following generic type:

type NotStrictProps<T> = T & { [key: string]: any }

then you should be able to use:

interface IAccordionPropsStrict { /** An element type to render as (string or function). */ as?: any } type IAccordionProps = NotStrictProps<IAccordionPropsStrict> const a: IAccordionProps = {} a.as = 'div' a.whatever = 'works as well'

Sure, as this is going to be used in all components, it would be good to be extracted as separate type. Thanks!

miroslavstastny · 2018-08-23T13:14:01Z

src/components/Accordion/Accordion.tsx

@@ -8,12 +8,59 @@ import AccordionContent from './AccordionContent'
 import { DefaultBehavior } from '../../lib/accessibility'
 import { Accessibility } from '../../lib/accessibility/interfaces'

+export interface IAccordionPropsStrict {
+  /** An element type to render as (string or function). */
+  as?: any


I would make a separate interface for as. Then it could be used in getElementType()

miroslavstastny · 2018-08-23T13:16:57Z

src/components/Accordion/Accordion.tsx

+  }[]
+
+  /** Accessibility behavior if overridden by the user. */
+  accessibility?: object | Function


accessibility, styles, variables, className are common for all components, shouldn't we extract them to separate interface. If so, then to how many?
Does the same apply to children?

I was thinking about the same thing. For the children, they should be provided even from React itself so I am not sure if we should specify them here at all. Not sure if the same applies for the className as well. For the rest, I would propose having the styles and variables in one interface, because for all themed components both would apply, and having the accessibility in other interface, because I am not sure if it is applicable to all components. We should think about the 'as' property as well. Having this said, I am kind a worry that we would end up with lot's of interfaces, which can become confusing, but at the same time I agree that it could be good for refactoring, if we need to add some common property or rename existing common property in the future for some reason. Any thoughts on this? @kuzhelov @levithomason

agree with the points mentioned above. However, my vote would rather be to do it after strict rtpes will be introduced for all components, so that at this point we will have clear picture around what are the general parts and what are the exceptions/edge cases.

Got it! Will wait then for final approve for this PR in order for it to be merged.

miroslavstastny · 2018-08-23T13:17:37Z

src/components/Accordion/Accordion.tsx

+   * @param {SyntheticEvent} event - React's original SyntheticEvent.
+   * @param {object} data - All item props.
+   */
+  onTitleClick?: Function


We should be more specific with Function types.

…perty

smykhailov · 2018-08-23T13:23:03Z

src/components/Accordion/Accordion.tsx

+  variables?: object | Function
+}
+
+export interface IAccordionProps extends IAccordionPropsStrict {


actually it can be done in more generic way

type Indexer<T> = { [key: string]: T } interface IAccordionPropsStrict { as?: string ... } interface IAccordionProps extends Indexer<any>, IAccordionPropsStrict { }

I like these approach as being more generic, but I think the NotStrictProps types defined as is is sufficient enough and is more explicit with it's name when defining the not strict props interfaces. I will change the definition if you think that we should go with the Indexer type.

bmdalex · 2018-08-23T12:43:27Z

src/components/Accordion/Accordion.tsx

+  accessibility?: object | Function
+
+  /** Custom styles to be applied for component. */
+  styles?: object | Function


we can use ICSSInJSStyle to get better typings (instead of object)

/** Custom styles to be applied for component. */ styles?: ICSSInJSStyle | Function

what do u think?

Actually this would be the following form {root: () => ICSSInJSStyle, otherPart: () => ICSSInJSStyle...} so we cannot generalize to just ICSSInJSStyle

Talking about styles, we may be able to define the variables and styles in the following way:
variables?: ComponentVariablesInput
styles?: IComponentPartStylesInput
as these are the types used in the renderComponent. @levithomason am I right about this?

bmdalex · 2018-08-23T12:44:48Z

src/components/Accordion/Accordion.tsx

+  variables?: object | Function
+}
+
+export interface IAccordionProps extends IAccordionPropsStrict {


can we add this as a prop to extend from in UIComponent.tsx together with the other common props? (as, styles and variables)?

something like :

export interface UIComponentProps { [key: string]: any /** An element type to render as (string or function). */ as?: any /** Custom styles to be applied for component. */ styles?: ICSSInJSStyle | Function /** Custom variables to be applied for component. */ variables?: object | Function } class UIComponent<P, S> extends React.Component<P & UIComponentProps, S> { // ... }

and then we remove these from the particular component interfaces

@mnajdova @kuzhelov @levithomason

kuzhelov · 2018-08-24T09:35:49Z

agreed to decide on where exactlyNotStrictProps<T> should be placed later - currently leave it in the suggested by this PR place

fix: Component interfaces extend React attributes

6028777

bmdalex requested review from dvdzkwsk, kuzhelov, levithomason and mnajdova as code owners July 31, 2018 17:42

bmdalex changed the title ~~fix: Component interfaces extend React attributes~~ [READY FOR REVIEW] fix: Component interfaces extend React attributes Jul 31, 2018

manajdov added 7 commits August 1, 2018 10:53

-fixing types

7813cac

-fixed images in the avatar

1616e3c

-added variables prop to the Label component

b2b0f87

-added props interface to the Label component

799677e

-fixes

4800164

-reverted changes in the avatar examples

0fc3605

Merge branch 'master' into fix/component-interfaces

3615ba7

kuzhelov approved these changes Aug 1, 2018

View reviewed changes

kuzhelov changed the title ~~[READY FOR REVIEW] fix: Component interfaces extend React attributes~~ [CONFIRM MERGE] fix: Component interfaces extend React attributes Aug 1, 2018

kuzhelov changed the title ~~[CONFIRM MERGE] fix: Component interfaces extend React attributes~~ [ON REVIEW] fix: Component interfaces extend React attributes Aug 1, 2018

kuzhelov reviewed Aug 1, 2018

View reviewed changes

manajdov added 2 commits August 1, 2018 19:01

-fixed variables type to work with object too

9a39575

-changed the PropTypes in the Label component

c2e377f

levithomason reviewed Aug 2, 2018

View reviewed changes

kuzhelov changed the title ~~[ON REVIEW] fix: Component interfaces extend React attributes~~ [POSTPONED] fix: Component interfaces extend React attributes Aug 2, 2018

mnajdova mentioned this pull request Aug 2, 2018

fix: removed interfaces for the props in the components #43

Merged

levithomason mentioned this pull request Aug 20, 2018

Add typings for component props #117

Closed

19 tasks

levithomason changed the title ~~[POSTPONED] fix: Component interfaces extend React attributes~~ feat(Accordion): add props interface Aug 20, 2018

kuzhelov added the needs author changes Author needs to implement changes before merge label Aug 22, 2018

manajdov added 2 commits August 23, 2018 13:40

Merge branch 'master' into fix/component-interfaces

c7979b6

# Conflicts: # src/components/Avatar/avatarRules.ts # src/components/Button/Button.tsx # src/components/Icon/Icon.tsx # src/components/Label/Label.tsx # src/components/Text/Text.tsx # src/lib/UIComponent.tsx

-added props interface for the accordion component

da7abe4

bmdalex requested a review from miroslavstastny as a code owner August 23, 2018 12:25

jurokapsiar reviewed Aug 23, 2018

View reviewed changes

smykhailov approved these changes Aug 23, 2018

View reviewed changes

miroslavstastny reviewed Aug 23, 2018

View reviewed changes

manajdov added 2 commits August 23, 2018 15:27

-extracting NotStringProps generic type

21f79e4

-more specific function definition for the Accordion onTitleClick pro…

bc5fdd3

…perty

smykhailov reviewed Aug 23, 2018

View reviewed changes

mnajdova added 🚀 ready for review and removed needs author changes Author needs to implement changes before merge labels Aug 23, 2018

bmdalex commented Aug 23, 2018

View reviewed changes

manajdov added 2 commits August 24, 2018 11:10

-updated types for the variables and styles

a517d51

-removing children from prop interface

276a8c6

kuzhelov approved these changes Aug 24, 2018

View reviewed changes

Merge branch 'master' into fix/component-interfaces

aa26eb0

mnajdova merged commit 4ebb956 into master Aug 24, 2018

bmdalex deleted the fix/component-interfaces branch September 7, 2018 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Accordion): add props interface #33

feat(Accordion): add props interface #33

bmdalex commented Jul 31, 2018

codecov bot commented Jul 31, 2018 •

edited

Loading

bmdalex commented Jul 31, 2018

kuzhelov left a comment

kuzhelov Aug 1, 2018

kuzhelov Aug 1, 2018

kuzhelov Aug 1, 2018

kuzhelov Aug 1, 2018

mnajdova Aug 1, 2018

levithomason Aug 2, 2018

levithomason Aug 2, 2018

levithomason commented Aug 2, 2018 •

edited

Loading

mnajdova commented Aug 2, 2018

kuzhelov commented Aug 2, 2018 •

edited

Loading

levithomason commented Aug 6, 2018

mnajdova commented Aug 8, 2018

levithomason commented Aug 20, 2018

levithomason commented Aug 20, 2018

mnajdova commented Aug 23, 2018 •

edited

Loading

jurokapsiar Aug 23, 2018 •

edited

Loading

mnajdova Aug 23, 2018

miroslavstastny Aug 23, 2018

miroslavstastny Aug 23, 2018

mnajdova Aug 23, 2018 •

edited

Loading

kuzhelov Aug 23, 2018

mnajdova Aug 23, 2018

miroslavstastny Aug 23, 2018

mnajdova Aug 23, 2018

smykhailov Aug 23, 2018

mnajdova Aug 23, 2018

bmdalex Aug 23, 2018

mnajdova Aug 23, 2018

mnajdova Aug 23, 2018

bmdalex Aug 23, 2018

kuzhelov commented Aug 24, 2018

feat(Accordion): add props interface #33

feat(Accordion): add props interface #33

Conversation

bmdalex commented Jul 31, 2018

Component Interfaces

codecov bot commented Jul 31, 2018 • edited Loading

Codecov Report

bmdalex commented Jul 31, 2018

kuzhelov left a 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

levithomason commented Aug 2, 2018 • edited Loading

mnajdova commented Aug 2, 2018

kuzhelov commented Aug 2, 2018 • edited Loading

levithomason commented Aug 6, 2018

mnajdova commented Aug 8, 2018

levithomason commented Aug 20, 2018

levithomason commented Aug 20, 2018

mnajdova commented Aug 23, 2018 • edited Loading

jurokapsiar Aug 23, 2018 • edited Loading

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

mnajdova Aug 23, 2018 • edited Loading

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 commented Aug 24, 2018

codecov bot commented Jul 31, 2018 •

edited

Loading

levithomason commented Aug 2, 2018 •

edited

Loading

kuzhelov commented Aug 2, 2018 •

edited

Loading

mnajdova commented Aug 23, 2018 •

edited

Loading

jurokapsiar Aug 23, 2018 •

edited

Loading

mnajdova Aug 23, 2018 •

edited

Loading