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

feat(Accordion): add props interface #33

Merged
merged 17 commits into from
Aug 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion src/components/Accordion/Accordion.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,57 @@ import AccordionTitle from './AccordionTitle'
import AccordionContent from './AccordionContent'
import { DefaultBehavior } from '../../lib/accessibility'
import { Accessibility } from '../../lib/accessibility/interfaces'
import { NotStrictProps } from '../../lib/NotStrictProps'
import { ComponentVariablesInput, IComponentPartStylesInput } from '../../../types/theme'

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

/** Index of the currently active panel. */
activeIndex?: number[] | number

/** Additional classes. */
className?: string

/** Initial activeIndex value. */
defaultActiveIndex?: number[] | number

/** Only allow one panel open at a time. */
exclusive?: boolean

/**
* Called when a panel title is clicked.
*
* @param {SyntheticEvent} event - React's original SyntheticEvent.
* @param {object} data - All item props.
*/
onTitleClick?: (event: React.SyntheticEvent, data: IAccordionProps) => void

/** Shorthand array of props for Accordion. */
panels?: {
content: React.ReactNode | object
title: React.ReactNode | object
}[]

/** Accessibility behavior if overridden by the user. */
accessibility?: object | Function
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@mnajdova mnajdova Aug 23, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


/** Custom styles to be applied for component. */
styles?: IComponentPartStylesInput

/** Custom variables to be applied for component. */
variables?: ComponentVariablesInput
}

export type IAccordionProps = NotStrictProps<IAccordionPropsStrict>

/**
* A standard Accordion.
* @accessibility
* Concern: how do we optimally navigate through an Accordion element with nested children?
*/
class Accordion extends AutoControlledComponent<any, any> {
class Accordion extends AutoControlledComponent<IAccordionProps, any> {
static displayName = 'Accordion'

static className = 'ui-accordion'
Expand Down
3 changes: 3 additions & 0 deletions src/lib/NotStrictProps.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export type NotStrictProps<T> = T & {
[key: string]: any
}