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

fix(Accordian Indicator): Updating themeing #939

Closed
wants to merge 7 commits into from
Closed

fix(Accordian Indicator): Updating themeing #939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bfee520
adding more descriptive text for accordion
bcalvery Feb 4, 2019
69c97d0
Merge branch 'master' of https://github.com/stardust-ui/react into fi…
bcalvery Feb 4, 2019
a78624e
Merge branch 'master' of https://github.com/stardust-ui/react into fi…
bcalvery Feb 15, 2019
9211da6
small check-ins for changing icon and hover behavior.
bcalvery Feb 15, 2019
8033e7c
changes to add icon to accordion
bcalvery Feb 20, 2019
1040baa
Merge branch 'master' of https://github.com/stardust-ui/react into fi…
bcalvery Feb 20, 2019
274f70e
add changelog entry
bcalvery Feb 20, 2019
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Display correctly images in portrait mode inside `Avatar` @layershifter ([#899](https://github.com/stardust-ui/react/pull/899))
- Expose `Popup`'s content Ref @sophieH29 ([#913](https://github.com/stardust-ui/react/pull/913))
- Fix `Button` Teams theme styles to use semibold weight @notandrew ([#829](https://github.com/stardust-ui/react/pull/829))
- Fix `Accordion` and `Indicator` Themeing update @bcalvery ([#939](https://github.com/stardust-ui/react/pull/939))

<!--------------------------------[ v0.21.1 ]------------------------------- -->
## [v0.21.1](https://github.com/stardust-ui/react/tree/v0.21.1) (2019-02-14)
Expand Down
2 changes: 1 addition & 1 deletion docs/src/examples/components/Accordion/Types/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const Types = () => (
/>
<ComponentExample
title="Exclusive"
description="An exclusive Accordion."
description="An exclusive Accordion can only have one item open at a time."
examplePath="components/Accordion/Types/AccordionExclusiveExample"
/>
</ExampleSection>
Expand Down
23 changes: 23 additions & 0 deletions docs/src/examples/components/Accordion/Usage/AccordionPanelCustomTitleExample.shorthand.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,29 @@ class AccordionPanelCustomTitleExample extends React.Component {
content: 'Here is a list of warnings discovered.',
},
},
{
title: {
content: (
<div>
<strong>Someone</strong> has added <strong>Someone Else</strong> to the team
</div>
),
icon: { name: 'participant-add', variables: { outline: true } },
},
content: {
key: 'peopleadded',
content: (
<div>
<div style={{ padding: '0 0 .5em 0' }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

please do not change example styles to match specific theme;

I'd suggest reverting these changes in this file and create a prototype (use #931 as a reference on how to create one)

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about creating a new example under Usages? If it's an example, then screenshot tests will capture any regressions - I don't think prototypes have that same benefit

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about creating a new example under Usages?

I also don't think it's a good practice.

The point of the Examples section is to provide simple and generic code snippets to show clients how to use our components. It is not intended to show them a very particular set of customizations to achieve a very particular flavor for that component.

If it's an example, then screenshot tests will capture any regressions - I don't think prototypes have that same benefit

They don't but these screenshots tests are not meant to capture regressions for styles inserted in the examples themselves, but for the whole theme (so for styles that are introduced in the top level theme object for the theme that is currently displayed in the docs site). Hope this makes sense.

<strong>Someone</strong> has added <strong>Someone Else</strong> to the team
</div>
<div style={{ padding: '0 0 .5em 0' }}>
<strong>Someone</strong> has added <strong>Someone Else</strong> to the team
</div>
</div>
),
},
},
]

return <Accordion defaultActiveIndex={[0]} panels={panels} />
Expand Down
6 changes: 5 additions & 1 deletion packages/react/src/components/Accordion/Accordion.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ class Accordion extends AutoControlledComponent<ReactProps<AccordionProps>, any>
_.each(panels, (panel, index) => {
const { content, title } = panel
const active = this.isIndexActive(index)
const indented = title.hasOwnProperty('icon')
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a very specific detail and does not look like functionality that is generally applicable for any Accordion; can you achieve this on the theme side (packages/react/src/themes/teams/components/Accordion/accordionStyles.ts)


children.push(
AccordionTitle.create(title, {
Expand All @@ -167,7 +168,10 @@ class Accordion extends AutoControlledComponent<ReactProps<AccordionProps>, any>
)
children.push(
AccordionContent.create(content, {
defaultProps: { active },
defaultProps: {
active,
indented,
},
render: renderPanelContent,
}),
)
Expand Down
4 changes: 4 additions & 0 deletions packages/react/src/components/Accordion/AccordionContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ export interface AccordionContentProps
/** Whether or not the content is visible. */
active?: boolean

/** Whether or not content should be indented */
indented?: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please:

  • explain why this prop is relevant in describing the content of an accordion?
  • describe the scenario where this prop is needed?

We need a strong argument with examples (preferably from the larger web community) that a prop is describing a component before we can accept it as a valid prop

@kuzhelov
@layershifter
@miroslavstastny
@mnajdova
@alinais

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I definitely don't like the feeling of having this prop. It is an artifact of the way Accordion renders the Title and Content of each panel separately within the Accordion. The purpose of Indented is to react to the presence of the Icon (that is a question in itself) and indent the accordion content accordingly.
With and without Icon:
image


/**
* Called on click.
*
Expand All @@ -43,6 +46,7 @@ class AccordionContent extends UIComponent<ReactProps<AccordionContentProps>, an
...commonPropTypes.createCommon(),
active: PropTypes.bool,
onClick: PropTypes.func,
indented: PropTypes.bool,
}

renderComponent({ ElementType, classes, unhandledProps }) {
Expand Down
25 changes: 18 additions & 7 deletions packages/react/src/components/Accordion/AccordionTitle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
rtlTextContainer,
} from '../../lib'
import { ReactProps, ComponentEventHandler, ShorthandValue } from '../../types'
import Icon from '../Icon/Icon'
import Indicator from '../Indicator/Indicator'
import Layout from '../Layout/Layout'

Expand All @@ -37,6 +38,9 @@ export interface AccordionTitleProps

/** Shorthand for the active indicator. */
indicator?: ShorthandValue

/** Shorthand for the icon. */
icon?: ShorthandValue
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kuzhelov
@layershifter
@miroslavstastny
@mnajdova
@alinais

do we have any agreement on the icon prop for Accordion?

Copy link
Member

Choose a reason for hiding this comment

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

We should not introduce a new slot there, Indicator already covers it:

image
https://codesandbox.io/s/n725p7rz54?module=/example.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Icon is intended to be separate form the indicator. But maybe Adding Icon in general is not right. Maybe making Accordion usable for the control messages was the wrong direction?

}

/**
Expand All @@ -55,24 +59,31 @@ class AccordionTitle extends UIComponent<ReactProps<AccordionTitleProps>, any> {
index: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
onClick: PropTypes.func,
indicator: customPropTypes.itemShorthand,
icon: customPropTypes.itemShorthand,
}

handleClick = e => {
_.invoke(this.props, 'onClick', e, this.props)
}

renderComponent({ ElementType, classes, unhandledProps, styles }) {
const { children, content, indicator, active } = this.props
const { children, content, icon, indicator, active } = this.props
const indicatorWithDefaults = indicator === undefined ? {} : indicator
const indicatorIcon = icon === undefined ? {} : icon

const contentElement = (
<Layout
start={Indicator.create(indicatorWithDefaults, {
defaultProps: {
direction: active ? 'bottom' : 'end',
styles: styles.indicator,
},
})}
start={
<Layout
Copy link
Collaborator

Choose a reason for hiding this comment

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

please do not use Layout component as it's going to be deprecated and it renders a lot of extra DOM nodes; take a look at Flex component as it might solve your scenario

start={Indicator.create(indicatorWithDefaults, {
defaultProps: {
direction: active ? 'bottom' : 'end',
styles: styles.indicator,
},
})}
main={indicatorIcon ? Icon.create(indicatorIcon) : null}
/>
}
main={rtlTextContainer.createFor({ element: content })}
/>
)
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/components/Indicator/Indicator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class Indicator extends UIComponent<ReactProps<IndicatorProps>, any> {
accessibility: defaultBehavior,
as: 'span',
direction: 'bottom',
icon: 'triangle-down',
}

renderComponent({ accessibility, ElementType, classes, unhandledProps, rtl }) {
Expand Down
5 changes: 5 additions & 0 deletions packages/react/src/themes/teams/components/Accordion/accordionContentStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@ const accordionContentStyles = {
root: ({ props }): ICSSInJSStyle => ({
display: 'none',
verticalAlign: 'middle',
marginLeft: '24px',
...(props.active && { display: 'block' }),
...(props.indented && {
// marginLeft should be IndicatorWidth + IconWidth
marginLeft: '48px',
}),
}),
}

Expand Down
15 changes: 13 additions & 2 deletions packages/react/src/themes/teams/components/Accordion/accordionTitleStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,23 @@
import { ICSSInJSStyle } from '../../../types'
import Indicator from '../../../../components/Indicator/Indicator'

const accordionTitleStyles = {
root: () => ({
root: (): ICSSInJSStyle => ({
display: 'inline-block',
verticalAlign: 'middle',
padding: '.5rem 0',
cursor: 'pointer',

':hover': {
[`& .${Indicator.className}`]: {
opacity: 1,
},
},
}),
indicator: () => ({
indicator: (): ICSSInJSStyle => ({
marginTop: '-.4rem',
userSelect: 'none',
opacity: 0.5,
}),
}

Expand Down
11 changes: 0 additions & 11 deletions packages/react/src/themes/teams/components/Icon/svg/ProcessedIcons/icons-indicator.tsx
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions packages/react/src/themes/teams/components/Icon/svg/ProcessedIcons/index-new.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,6 @@ import processedIcons_inputvalid from './icons-input-valid'
import processedIcons_inputinvalid from './icons-input-invalid'
import processedIcons_info from './icons-info'
import processedIcons_inferred from './icons-inferred'
import processedIcons_indicator from './icons-indicator'
import processedIcons_indent from './icons-indent'
import processedIcons_image from './icons-image'
import processedIcons_horizontalrule from './icons-horizontal-rule'
Expand Down Expand Up @@ -460,7 +459,6 @@ export default {
processedIcons_inputinvalid,
processedIcons_info,
processedIcons_inferred,
processedIcons_indicator,
processedIcons_indent,
processedIcons_image,
processedIcons_horizontalrule,
Expand Down
2 changes: 0 additions & 2 deletions packages/react/src/themes/teams/components/Icon/svg/ProcessedIcons/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ import processedIcons_inputvalid from './icons-input-valid'
import processedIcons_inputinvalid from './icons-input-invalid'
import processedIcons_info from './icons-info'
import processedIcons_inferred from './icons-inferred'
import processedIcons_indicator from './icons-indicator'
import processedIcons_indent from './icons-indent'
import processedIcons_image from './icons-image'
import processedIcons_horizontalrule from './icons-horizontal-rule'
Expand Down Expand Up @@ -461,7 +460,6 @@ export default {
processedIcons_inputinvalid,
processedIcons_info,
processedIcons_inferred,
processedIcons_indicator,
processedIcons_indent,
processedIcons_image,
processedIcons_horizontalrule,
Expand Down