Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Allows to experiment autoHeight behavior #3216

Merged
merged 2 commits into from
Nov 30, 2021

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Nov 18, 2021

@flaviendelangle
Copy link
Member

@alexfauquette any idea why you have to many visual diff on the pagination footer ?

@alexfauquette
Copy link
Member Author

No, I'm only modifying a doc example, so it should not impact other screenshots. Probably my branch is too old. I merge upstream/master, we will see

@alexfauquette alexfauquette merged commit 570b58d into mui:master Nov 30, 2021
@alexfauquette alexfauquette deleted the doc-fixes branch November 30, 2021 13:59
@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation labels Nov 30, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

👍

maxColumns: 6,
});

return (
<div style={{ height: 400, width: '100%' }}>
<DataGrid autoHeight {...data} />
<p>more content</p>
Copy link
Member

Choose a reason for hiding this comment

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

For context, I added this node because the first implement of autoHeight got it wrong, <p>more content</p> was displayed on top of the data grid 😆. I used the visual regression test for this, I'm not sure there are any other tests: #981 (comment). I guess we can wait for a regression that might never happen to reconsider the approach.

Comment on lines +24 to +25
</Button>
<DataGrid autoHeight {...data} rows={data.rows.slice(0, nbRows)} />
Copy link
Member

Choose a reason for hiding this comment

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

It could have been nice to add some spacing between the two:

Screenshot 2021-11-30 at 17 31 30

Copy link
Member

Choose a reason for hiding this comment

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

I usually do the following

      <Stack
        sx={{ width: '100%', mb: 1 }}
        direction="row"
        alignItems="flex-start"
        columnGap={1}
      >
       // My buttons
      </Stack>

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, between the 3 elements then 😁 . One constraint here is that I think we need to make the autoHeight prop easy to spot for developers, not to get them flooded with other concerns.

Copy link
Member

Choose a reason for hiding this comment

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

@flaviendelangle thoughts:

  • columnGap={1}. why not use the spacing prop? AFAIK the flexbox gap prop that is not compatible with our supported browsers (Safari).
  • "I usually do the following" 👍 for normalizing the demos so we can say "we do x" :) for instance, outlined button vs. text ones
  • alignItems="flex-start" do we really need this one? I tried without it and couldn't spot an issue.
  • width: '100%' I imagine that as a div inside a parent that is width: 100% we can remove it

Copy link
Member

Choose a reason for hiding this comment

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

why not use the spacing prop

Because I did not know it existed 😄

I'll trying improving mine and applying it on other demos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants