fix(Accordian Indicator): Updating themeing #939
Conversation
…x/accordian-themeing
…x/accordian-themeing
…x/accordian-themeing
bcalvery
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #939 +/- ##
==========================================
- Coverage 80.51% 80.49% -0.02%
==========================================
Files 659 658 -1
Lines 8447 8449 +2
Branches 1492 1495 +3
==========================================
Hits 6801 6801
- Misses 1631 1634 +3
+ Partials 15 14 -1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #939 +/- ##
==========================================
- Coverage 80.51% 80.49% -0.02%
==========================================
Files 659 658 -1
Lines 8447 8449 +2
Branches 1492 1431 -61
==========================================
Hits 6801 6801
- Misses 1631 1634 +3
+ Partials 15 14 -1
Continue to review full report at Codecov.
|
| }, | ||
| })} | ||
| start={ | ||
| <Layout |
There was a problem hiding this comment.
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
| key: 'peopleadded', | ||
| content: ( | ||
| <div> | ||
| <div style={{ padding: '0 0 .5em 0' }}> |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
| @@ -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') | |||
There was a problem hiding this comment.
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)
| @@ -37,6 +38,9 @@ export interface AccordionTitleProps | |||
|
|
|||
| /** Shorthand for the active indicator. */ | |||
| indicator?: ShorthandValue | |||
|
|
|||
| /** Shorthand for the icon. */ | |||
| icon?: ShorthandValue | |||
There was a problem hiding this comment.
@kuzhelov
@layershifter
@miroslavstastny
@mnajdova
@alinais
do we have any agreement on the icon prop for Accordion?
There was a problem hiding this comment.
We should not introduce a new slot there, Indicator already covers it:
There was a problem hiding this comment.
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?
Latest changes:
Goal to match Teams control messages:
Issues:
Keyboard navigation
Content Title content isn't actionable
Need to support an empty state (and hide Indicator when empty)