feat(CollapsiblePanel): add `hideExpansionControls` (not hidden by default) #421

tdeekens · 2019-01-10T09:08:56Z

Summary

This pull request adds an hideExpansionControls to the <CollapiblePanel /> while (by default) showing the expansion controls in disabled visuals when the panel itself is disabled.

Description

The images visualises the change. The change itself is reasoned in not having the content in the header of the panel "shift" or move when disabling or enabling it. So a previously clickable part remains clickable and does not move away underneath a user's mouse.

…abled

…s and disabled

tdeekens · 2019-01-10T09:10:30Z

src/components/panels/collapsible-panel/collapsible-panel.js

-            })}
+            className={classnames(
+              this.props.className,
+              styles[`container-theme-${this.props.theme}`],


A similar classname was built up by convention elsewhere in the file. Doing the same here shortens the classnames "block".

tdeekens · 2019-01-10T09:10:50Z

src/components/panels/collapsible-panel/collapsible-panel.js

                  <div className={styles['truncate-header']}>
                    <Spacings.Inline alignItems="center" scale="s">
-                      {!this.props.isDisabled && (
+                      {!this.props.hideExpansionControls && (


The actual change. Instead of hiding them when disabled we show them.

montezume · 2019-01-10T09:23:45Z

@tdeekens can you ask @mariabarrena to review the percy diff?

tdeekens · 2019-01-10T09:42:22Z

@tdeekens can you ask @mariabarrena to review the percy diff?

Yup was about to.

montezume · 2019-01-10T09:45:20Z

src/components/panels/collapsible-panel/README.md

+| `description`           | `String` |                                                          | -                     |           | If passed will be shown below the title as more information regarding the panel                                                           |
+| `isDisabled`            | `bool`   |                                                          | -                     | false     | Disables the panel and all interactions with it                                                                                           |
+| `children`              | `node`   |                                                          | -                     | -         | The actual content rendered inside the panel                                                                                              |
+| `hideExpansionControls` | `bool`   |                                                          | -                     | -         | Controls the visibility of the expansion controls on the left                                                                             |


I'm not sure about this prop name. @dferber90 any ideas?

It's an attempt to align it with some inputs which also have an option to be expanded. The intend was to have transferable vocabulary also across components of different groups. Let me know if you come up with a better name and the reasoning for it.

but we no longer have any other props called hideExpansionControls, they were changed here.

They're called hideCurrencyExpansionControls or hideLanguageExpansionControls. I presume you want it more specific? hidePanelExpansionControls maybe? Your call, gimme a name :)

Well in this case you are expanding or collapsing... maybe the name is correct after all 🤷‍♂️

Just a note this is just in case the component is disabled right? So we could assume that you set it whenever is disabled meaning that the panel is collapsed so now you can only expand 🤔

I don't really have strong opinions on it. hideExpansionControls is fine by me. 🤷‍♂️

I will wait for the @dferber90 to give his feedback for this.

I'm fine with this name 👍

We used more specific names (defaultExpandMultilineText, defaultExpandLanguages) in LocalizedMultilineTextInput as there are two things which can be expanded/collapsed. Here it's only one thing, so hideExpansionControls seems fine. On a sidenote, LocalizedMultilineTextInput only accepts hideLanguageExpansionControls as the multiline expansion controls can't be hidden (yet).

Sidenote
LocalizedMultilineTextInput always acts as an uncontrolled component regarding its expansion states (it doesn't accept the state from the parent). Therefore it doesn't accept props like expandMultilineText or expandLanguages (or isMultilineTextExpanded and areLanguagesExpanded).

For the component in this PR, I slightly dislike that hideExpansionControls and isClosed don't use the same terminology (open/closed vs expanded/collapsed). We could do hideExpansionControls and isExpanded so that they use the same terminology for the same concept which shows that the props belong together. But that's probably not worth the refactoring as we'd also need to flip the default. So let's just keep isClosed and add hideExpansionControls.

mariabarrena · 2019-01-10T09:51:18Z

It looks exactly how it was designed. Thanks!
Before approve it on percy, I want to double check with any of you one thing. In the screenshot I see some of the possible states of the panel being showed on light and dark themes (f.e. "urgent and dark" and "urgent and light") However for disabled I just see one theme being showed. We definitely need the disabled and the hide expansion control states for both themes. Do we need to show both on percy?

montezume · 2019-01-10T09:56:31Z

@mariabarrena sure, it makes sense to visually test all the possible iterations 👍 .

tdeekens · 2019-01-10T09:59:12Z

Do we need to show both on percy?

Sure, I'll add them. I just tried to keep it a bit more minimal. I'll add all variations coming to mind.

tdeekens · 2019-01-10T10:37:29Z

Alright @mariabarrena I added more visual specs. Adding all is close to bit silly as it's quite a bit of combinations. I added at least one per case and "combination variation". So condensed is disabled and urgent too yadada.

Ref to Percy

qmateub · 2019-01-10T10:29:42Z

src/components/panels/collapsible-panel/README.md

+| `description`           | `String` |                                                          | -                     |           | If passed will be shown below the title as more information regarding the panel                                                           |
+| `isDisabled`            | `bool`   |                                                          | -                     | false     | Disables the panel and all interactions with it                                                                                           |
+| `children`              | `node`   |                                                          | -                     | -         | The actual content rendered inside the panel                                                                                              |
+| `hideExpansionControls` | `bool`   |                                                          | -                     | -         | Controls the visibility of the expansion controls on the left                                                                             |


Well in this case you are expanding or collapsing... maybe the name is correct after all 🤷‍♂️

Just a note this is just in case the component is disabled right? So we could assume that you set it whenever is disabled meaning that the panel is collapsed so now you can only expand 🤔

qmateub · 2019-01-10T10:30:16Z

src/components/panels/collapsible-panel/README.md

+| `hideExpansionControls` | `bool`   |                                                          | -                     | -         | Controls the visibility of the expansion controls on the left                                                                             |
+| `headerControls`        | `node`   |                                                          | -                     | -         | Controls at the top right part of the panel                                                                                               |
+| `tone`                  | `oneOf`  |                                                          | ['urgent', 'primary'] | 'primary' | -                                                                                                                                         |
+| `className`             | `bool`   |                                                          | -                     | -         | -                                                                                                                                         |


Note in the scope of this PR obviously but this should go away so no custom CSS leaks into the component 😋

Yup. That would be a breaking change however.

Yeah, it would be more appropriate to create an issue for that.

qmateub

dferber90

Thanks!

montezume · 2019-01-10T12:10:28Z

@mariabarrena can you approve the percy diff so we can get this merged then? 😇

mariabarrena · 2019-01-10T12:36:53Z

"Condensed light", "Condensed light - hideExpansionControls" and "Condensed light is Disabled" are showing the dark theme and not the light one. Once that is fixed, we are good to go. I'll wait for the change to approve it.

tdeekens added 4 commits January 10, 2019 10:01

refactor(collapsible-panel): to show but disable header icon when dis…

beffb36

…abled

feat(collapsible-panel): add hideExpansionControls

d3adb55

chore(collapible-panel): simplify application for theme css

d5d524a

feat(collapsible-panel): add visual route specs for expansion control…

d738081

…s and disabled

tdeekens changed the title ~~Add hideExpansionControls and do not hide controls when disabled~~ [CollapsiblePanel]: Add hideExpansionControls and do not hide controls when disabled Jan 10, 2019

tdeekens changed the title ~~[CollapsiblePanel]: Add hideExpansionControls and do not hide controls when disabled~~ feat(CollapsiblePanel): add hideExpansionControls (not hidden by default) Jan 10, 2019

tdeekens commented Jan 10, 2019

View reviewed changes

fix(collapsible-panel): to default not to be disabled

dff4df4

tdeekens requested review from montezume, qmateub and dferber90 January 10, 2019 09:15

tdeekens added the 👨‍👩‍👦‍👦 Status: Dev Review label Jan 10, 2019

tdeekens self-assigned this Jan 10, 2019

tdeekens added the Semver: FEATURE label Jan 10, 2019

montezume reviewed Jan 10, 2019

View reviewed changes

qmateub reviewed Jan 10, 2019

View reviewed changes

montezume approved these changes Jan 10, 2019

View reviewed changes

qmateub approved these changes Jan 10, 2019

View reviewed changes

dferber90 approved these changes Jan 10, 2019

View reviewed changes

montezume added the 👨‍🎨 Status: UI/UX Review Requires review from design team label Jan 10, 2019

tdeekens removed the 👨‍👩‍👦‍👦 Status: Dev Review label Jan 10, 2019

feat(collapsible-panel): add more visual specs

02be63b

tdeekens force-pushed the feat/auto-close-collapsible-when-disabled branch from f9eafde to 02be63b Compare January 10, 2019 12:40

tdeekens merged commit 0bd7fa9 into master Jan 10, 2019

tdeekens deleted the feat/auto-close-collapsible-when-disabled branch January 10, 2019 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(CollapsiblePanel): add `hideExpansionControls` (not hidden by default) #421

feat(CollapsiblePanel): add `hideExpansionControls` (not hidden by default) #421

tdeekens commented Jan 10, 2019 •

edited

Loading

tdeekens Jan 10, 2019

tdeekens Jan 10, 2019

montezume commented Jan 10, 2019

tdeekens commented Jan 10, 2019

montezume Jan 10, 2019

tdeekens Jan 10, 2019

montezume Jan 10, 2019

tdeekens Jan 10, 2019 •

edited

Loading

qmateub Jan 10, 2019

montezume Jan 10, 2019

tdeekens Jan 10, 2019

dferber90 Jan 10, 2019

mariabarrena commented Jan 10, 2019

montezume commented Jan 10, 2019

tdeekens commented Jan 10, 2019 •

edited

Loading

tdeekens commented Jan 10, 2019 •

edited

Loading

qmateub Jan 10, 2019

qmateub Jan 10, 2019

tdeekens Jan 10, 2019

montezume Jan 10, 2019

qmateub left a comment

dferber90 left a comment

montezume commented Jan 10, 2019

mariabarrena commented Jan 10, 2019

feat(CollapsiblePanel): add hideExpansionControls (not hidden by default) #421

feat(CollapsiblePanel): add hideExpansionControls (not hidden by default) #421

Conversation

tdeekens commented Jan 10, 2019 • edited Loading

Summary

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montezume commented Jan 10, 2019

tdeekens commented Jan 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdeekens Jan 10, 2019 • 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

mariabarrena commented Jan 10, 2019

montezume commented Jan 10, 2019

tdeekens commented Jan 10, 2019 • edited Loading

tdeekens commented Jan 10, 2019 • 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

qmateub left a comment

Choose a reason for hiding this comment

dferber90 left a comment

Choose a reason for hiding this comment

montezume commented Jan 10, 2019

mariabarrena commented Jan 10, 2019

feat(CollapsiblePanel): add `hideExpansionControls` (not hidden by default) #421

feat(CollapsiblePanel): add `hideExpansionControls` (not hidden by default) #421

tdeekens commented Jan 10, 2019 •

edited

Loading

tdeekens Jan 10, 2019 •

edited

Loading

tdeekens commented Jan 10, 2019 •

edited

Loading

tdeekens commented Jan 10, 2019 •

edited

Loading