[Collapse] Add orientation and horizontal support #20619

darkowic · 2020-04-18T11:22:12Z

Breaking changes

The collapsedHeight prop was renamed collapsedSize to support the horizontal direction.
```
-<Collapse collapsedHeight={40}>
+<Collapse collapsedSize={40}>
```

I have followed (at least) the PR section of the contributing guide.

One thing I am not sure about - how do we handle props deprecations? The yarn proptypes is not adding the deprecatedPropType util 🤔

Closes #10051

darkowic · 2020-04-18T11:23:27Z

Oh and another thing - I am not able to run tests because of following error:

$ yarn test:unit --grep Collapse
...
✖ ERROR: No valid exports main found for '/home/darkowic/projects/github/material-ui/node_modules/dom-accessibility-api'

mui-pr-bot · 2020-04-18T11:30:23Z

@material-ui/core: parsed: +0.13% , gzip: +0.20%
@material-ui/lab: parsed: +0.20% , gzip: +0.27%

Details of bundle changes

Generated by 🚫 dangerJS against 24c0cec

darkowic · 2020-04-18T14:05:54Z

There are still 2 things I am not sure about the collapse:

Why this component needs 2 wrappers?
IMHO the wrapper should set width and height to 100% to fill the whole root space:

after setting the wrapper's height to 100% (the content has height 100%):

eps1lon · 2020-04-19T11:02:30Z

Oh and another thing - I am not able to run tests because of following error:

What OS are you using and what's the output of node -v?

darkowic · 2020-04-19T11:22:17Z

Oh and another thing - I am not able to run tests because of following error:

What OS are you using and what's the output of node -v?

@eps1lon

$ uname -a
Linux pop-os 5.3.0-7648-generic #41~1586789791~19.10~9593806-Ubuntu SMP Mon Apr 13 17:50:40 UTC  x86_64 x86_64 x86_64 GNU/Linux
$ node -v
v13.5.0

darkowic · 2020-04-19T17:33:16Z

@oliviertassinari I see you added v5 label - when do you think we can release it?. Why not release it in some 4.x version? I would love to use it (I use it now - I copied the whole component into my source). I tried to not introduce any breaking changes.

oliviertassinari · 2020-04-19T17:40:48Z

@darkowic We are freezing all new features from the v4 branch.

darkowic · 2020-04-19T18:07:47Z

😒 Well, then I'm waiting for v5-alpha.1 🚀

mbrookes · 2020-06-22T19:03:21Z

@darkowic You can target the next branch with this.

darkowic · 2020-06-23T21:55:54Z

Updated ;) One unit tests is failing:

  it('can fallback to findDOMNode in a StrictMode theme', () => {

with TypeError: Cannot read property 'style' of null when trying to get wrapperRef.current and some browser tests are failing with similar error - any tips?

packages/material-ui/src/Collapse/Collapse.js

docs/src/pages/components/transitions/transitions.md

mbrookes · 2020-06-24T17:37:08Z

Marked as breaking in anticipation of the removal of collapsedHeight...

darkowic · 2020-06-26T20:33:56Z

btw yarn prettier command from contribution guide doesn't work

mbrookes · 2020-06-26T20:38:41Z

@darkowic What error are you getting?

darkowic · 2020-06-26T20:42:06Z

@mbrookes

[darkowic@pop-os] ~/projects/github/material-ui (10051_horizontal_collapse)
$ yarn prettier
yarn run v1.22.4
$ node ./scripts/prettier.js
prettier.js [mode]

formats codebase

Positionals:
  mode  "write" | "check-changed" | "write-changed"
                                             [string] [default: "write-changed"]

Options:
  --help  Show help                                                    [boolean]

Error: Command failed: git rev-parse next
fatal: ambiguous argument 'next': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

    at ChildProcess.exithandler (child_process.js:303:12)
    at ChildProcess.emit (events.js:315:20)
    at maybeClose (internal/child_process.js:1051:16)
    at Socket.<anonymous> (internal/child_process.js:442:11)
    at Socket.emit (events.js:315:20)
    at Pipe.<anonymous> (net.js:670:12) {
  killed: false,
  code: 128,
  signal: null,
  cmd: 'git rev-parse next',
  stdout: 'next\n',
  stderr: "fatal: ambiguous argument 'next': unknown revision or path not in the working tree.\n" +
    "Use '--' to separate paths from revisions, like this:\n" +
    "'git <command> [<revision>...] -- [<file>...]'\n"
}
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Fortunately, yarn prettier:all works ;)

mbrookes · 2020-06-27T12:21:37Z

Looks like you don't have a local next branch for listChangedFiles() to compare with. (Which pretter:all doesn't require.) Should probably have a defensive check...

mbrookes · 2020-06-27T12:32:21Z

Something's off with the horizontal collapse - it slides to the right rather than collapsing to the left when collapsedSize is set.

eps1lon · 2020-06-27T12:55:46Z

@darkowic Fetch to upstream remote again i.e. git fetch upstream if upstream is pointing to https://github.com/mui-org/material-ui.

darkowic · 2020-06-29T20:46:18Z

Something's off with the horizontal collapse - it slides to the right rather than collapsing to the left when collapsedSize is set.

@mbrookes hmm how it should work? Do we want to add some prop that defines which direction it is animated?

mbrookes · 2020-06-29T23:28:56Z

It should behave the same as without collapsedSize, but stop collapsing at collapsedSize width (as it does with vertical).

eps1lon · 2020-07-01T06:14:28Z

@darkowic Could you update this PR please with the latest version of the next branch?

darkowic · 2020-07-01T15:06:16Z

Yes, I will do it together with the fix/solution for collapse animation orientation

* BREAKING CHANGE: Remove collapsedHeight property - use collapsedSize (mui#10051)

darkowic · 2020-07-02T18:58:50Z

Rebased and improved demo. Actually it was looking weird because of space-between ;)

🚢 🎉

oliviertassinari · 2020-07-02T23:38:17Z

packages/material-ui/src/Collapse/Collapse.js

+ className={clsx(classes.wrapperInner, {
+ [classes.horizontal]: isHorizontal,
+ })}


What about we use Slider's approach?

https://github.com/mui-org/material-ui/blob/65d036e8aab5e0fe9e3efbf788daa8af8db57757/packages/material-ui/src/Slider/Slider.js#L192

Suggested change

className={clsx(classes.wrapperInner, {

[classes.horizontal]: isHorizontal,

})}

className={classes.wrapperInner}

oliviertassinari · 2020-07-02T23:42:05Z

packages/material-ui/src/Collapse/Collapse.js

@@ -41,15 +40,15 @@ export const styles = (theme) => ({
 display: 'flex',
 width: '100%',
 '&$horizontal': {
- width: 'initial',


https://stackoverflow.com/questions/20823105/use-initial-width-for-element-not-working-in-ie

oliviertassinari · 2020-07-02T23:42:31Z

packages/material-ui/src/Collapse/Collapse.js

@@ -11,13 +11,12 @@ import { useForkRef } from '../utils';
 export const styles = (theme) => ({
 /* Styles applied to the container element. */
 container: {
- position: 'relative',


Seem irrelevant

eps1lon · 2020-07-03T11:06:10Z

@darkowic Great work, thanks!

zachsa · 2020-07-27T11:37:14Z

Hi, I've been experimenting with this and I have found that the <Collapse /> component takes up the whole horizontal space with this structure:

<Grid container>
  <Collapse orientation="horizontal" in={showSidebar}>
    <Grid item xs={4}>
      <Sidebar />
    </Grid>
  </Collapse>

  <Grid item xs>
    <ResultList />
  </Grid>
</Grid>

In this case the Collapse component, when in = true, renders as 100% the width of the <Grid container>. I'm not actually sure that I'm using Grid correctly... But when I use any other Transition this is NOT the case. For example, replacing Collapse with

<Slide direction="right" unmountOnExit in={showSidebar}>
  ...
</Slide>

Then the layout behaves correctly (in that the slide component renders 'transparently', with the child Grid component sizing working as expected).

Looking at the HTML.

The Slide component gets classNames associated with the Grid (which I assume is the children?):

<div class="MuiGrid-root MuiGrid-item MuiGrid-grid-xs-4" style="transform: none; transition: transform 225ms cubic-bezier(0, 0, 0.2, 1) 0ms;" />

Whereas with the Collapse component, unlike with the Slide, no inline styles are giving, and the classes don't include any classes from the children:

<div class="MuiCollapse-container MuiCollapse-horizontal MuiCollapse-entered" style="min-width: 0px;">
  <div class="MuiCollapse-wrapper MuiCollapse-horizontal">
    <div class="MuiCollapse-wrapperInner MuiCollapse-horizontal">
...

Is this intended?

Rewriting my JSX:

<Grid container>
  <Collapse
    timeout={3000}
    className={clsx({
      'MuiGrid-root': true,
      'MuiGrid-item': true,
      'MuiGrid-grid-xs-4': showSidebar ? true : false,
    })}
    orientation="horizontal"
    in={showSidebar}
  >
    <Grid item xs={showSidebar ? 12 : 4}>
      <Sidebar />
    </Grid>
  </Collapse>

  <Grid item xs>
    <ResultList />
  </Grid>
</Grid>

This somewhat achieves what I'm looking for but in a very jumpy, unsatisfactory way (the timout={3000} allows me to see where the problems are).

I apologise if I'm misusing the Transition component in this case! If so, I would be super grateful for pointers in the correct direction.

eps1lon · 2020-07-27T12:37:56Z

@zachsa Please open a new issue and fill out the template. PRs are not monitored for potential issues.

zachsa · 2020-07-28T07:16:00Z

Thank you @eps1lon - I've written up an issue here: #21971

oliviertassinari added this to the v5 milestone Apr 19, 2020

oliviertassinari added the new feature New feature or request label Apr 19, 2020

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 20, 2020

darkowic force-pushed the 10051_horizontal_collapse branch from e88077a to 6a8f711 Compare June 23, 2020 21:29

darkowic requested a review from dimitropoulos as a code owner June 23, 2020 21:29

darkowic changed the base branch from master to next June 23, 2020 21:30

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 24, 2020

eps1lon self-assigned this Jun 24, 2020

eps1lon requested changes Jun 24, 2020

View reviewed changes

packages/material-ui/src/Collapse/Collapse.js Outdated Show resolved Hide resolved

packages/material-ui/src/Collapse/Collapse.js Outdated Show resolved Hide resolved

packages/material-ui/src/Collapse/Collapse.js Outdated Show resolved Hide resolved

mbrookes requested changes Jun 24, 2020

View reviewed changes

docs/src/pages/components/transitions/transitions.md Outdated Show resolved Hide resolved

docs/src/pages/components/transitions/transitions.md Outdated Show resolved Hide resolved

mbrookes added the breaking change label Jun 24, 2020

darkowic force-pushed the 10051_horizontal_collapse branch from 6a8f711 to e8f6272 Compare June 26, 2020 20:40

darkowic requested review from mbrookes and eps1lon June 27, 2020 07:08

eps1lon added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 1, 2020

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 1, 2020

[Collapse] Add orientation and horizontal support

77b487e

* BREAKING CHANGE: Remove collapsedHeight property - use collapsedSize (mui#10051)

darkowic force-pushed the 10051_horizontal_collapse branch from e8f6272 to 77b487e Compare July 2, 2020 18:57

mbrookes approved these changes Jul 2, 2020

View reviewed changes

eps1lon approved these changes Jul 2, 2020

View reviewed changes

oliviertassinari added 2 commits July 3, 2020 01:34

update migration guide

2eb8a8b

IE 11

24c0cec

oliviertassinari reviewed Jul 2, 2020

View reviewed changes

oliviertassinari removed the request for review from dimitropoulos July 2, 2020 23:43

oliviertassinari approved these changes Jul 2, 2020

View reviewed changes

eps1lon merged commit 64cbf68 into mui:next Jul 3, 2020

zachsa mentioned this pull request Jul 28, 2020

Collapse component doesn't wrap Grid items correctly #21971

Closed

2 tasks

eps1lon mentioned this pull request Aug 5, 2020

Deprecation warnings for v5 #22074

Closed

42 tasks

darkowic deleted the 10051_horizontal_collapse branch September 22, 2020 14:00

oliviertassinari mentioned this pull request Dec 20, 2020

[Collapse] Deprecate collapsedHeight #24079

Merged

mbrookes mentioned this pull request Dec 20, 2020

[RFC] Material-UI v5 🚀 #20012

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Collapse] Add orientation and horizontal support #20619

[Collapse] Add orientation and horizontal support #20619

darkowic commented Apr 18, 2020 •

edited by oliviertassinari

Loading

darkowic commented Apr 18, 2020

mui-pr-bot commented Apr 18, 2020 •

edited

Loading

darkowic commented Apr 18, 2020 •

edited

Loading

eps1lon commented Apr 19, 2020

darkowic commented Apr 19, 2020

darkowic commented Apr 19, 2020

oliviertassinari commented Apr 19, 2020

darkowic commented Apr 19, 2020

mbrookes commented Jun 22, 2020

darkowic commented Jun 23, 2020

mbrookes commented Jun 24, 2020

darkowic commented Jun 26, 2020

mbrookes commented Jun 26, 2020

darkowic commented Jun 26, 2020

mbrookes commented Jun 27, 2020

mbrookes commented Jun 27, 2020

eps1lon commented Jun 27, 2020

darkowic commented Jun 29, 2020

mbrookes commented Jun 29, 2020

eps1lon commented Jul 1, 2020

darkowic commented Jul 1, 2020

darkowic commented Jul 2, 2020

oliviertassinari Jul 2, 2020

oliviertassinari Jul 2, 2020

oliviertassinari Jul 2, 2020

eps1lon commented Jul 3, 2020

zachsa commented Jul 27, 2020

eps1lon commented Jul 27, 2020

zachsa commented Jul 28, 2020

[Collapse] Add orientation and horizontal support #20619

[Collapse] Add orientation and horizontal support #20619

Conversation

darkowic commented Apr 18, 2020 • edited by oliviertassinari Loading

Breaking changes

darkowic commented Apr 18, 2020

mui-pr-bot commented Apr 18, 2020 • edited Loading

darkowic commented Apr 18, 2020 • edited Loading

eps1lon commented Apr 19, 2020

darkowic commented Apr 19, 2020

darkowic commented Apr 19, 2020

oliviertassinari commented Apr 19, 2020

darkowic commented Apr 19, 2020

mbrookes commented Jun 22, 2020

darkowic commented Jun 23, 2020

mbrookes commented Jun 24, 2020

darkowic commented Jun 26, 2020

mbrookes commented Jun 26, 2020

darkowic commented Jun 26, 2020

mbrookes commented Jun 27, 2020

mbrookes commented Jun 27, 2020

eps1lon commented Jun 27, 2020

darkowic commented Jun 29, 2020

mbrookes commented Jun 29, 2020

eps1lon commented Jul 1, 2020

darkowic commented Jul 1, 2020

darkowic commented Jul 2, 2020

oliviertassinari Jul 2, 2020

Choose a reason for hiding this comment

oliviertassinari Jul 2, 2020

Choose a reason for hiding this comment

oliviertassinari Jul 2, 2020

Choose a reason for hiding this comment

eps1lon commented Jul 3, 2020

zachsa commented Jul 27, 2020

eps1lon commented Jul 27, 2020

zachsa commented Jul 28, 2020

darkowic commented Apr 18, 2020 •

edited by oliviertassinari

Loading

mui-pr-bot commented Apr 18, 2020 •

edited

Loading

darkowic commented Apr 18, 2020 •

edited

Loading