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.

Sign up for GitHub

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

Jump to bottom

[docs] Batch small changes #5177

Merged
merged 5 commits into from
Jun 14, 2022
Merged

[docs] Batch small changes #5177

merged 5 commits into from
Jun 14, 2022

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jun 11, 2022 •
edited
Loading

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Jun 11, 2022
@oliviertassinari oliviertassinari mentioned this pull request Jun 11, 2022
Merged
@mui-bot
Copy link

mui-bot commented Jun 11, 2022 •
edited
Loading

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 281.7 490.2 307.3 357.08 78.056
Sort 100k rows ms 461.7 914.1 697.3 719.06 156.848
Select 100k rows ms 156.9 305.6 183.7 202.66 54.008
Deselect 100k rows ms 120 208.6 176.7 168.7 33.623

Generated by 🚫 dangerJS against 68536ce

oliviertassinari
oliviertassinari commented Jun 11, 2022
View reviewed changes
packages/grid/x-data-grid/src/hooks/features/columns/useGridColumnSpanning.ts
@@ -1,4 +1,4 @@
import React from 'react';
import * as React from 'react';
Copy link
Member Author

Choose a reason for hiding this comment

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

Official import https://beta.reactjs.org/apis

packages/grid/x-data-grid/src/components/toolbar/GridToolbarFilterButton.tsx
@@ -119,7 +119,6 @@ const GridToolbarFilterButton = React.forwardRef<HTMLButtonElement, GridToolbarF
<rootProps.components.BaseButton
ref={ref}
size="small"
color="primary"
Copy link
Member Author

Choose a reason for hiding this comment

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

color primary is now the default in the minimum version of Material UI we depend on: https://mui.com/material-ui/migration/v5-component-changes/#%E2%9C%85-remove-default-color-prop

packages/grid/x-data-grid-generator/src/renderer/renderEditStatus.tsx
@@ -44,7 +44,7 @@ function EditStatus(props: GridRenderEditCellParams<string>) {
onClose: handleClose,
}}
sx={{
height: 1,
height: '100%',
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this is kind of confusing for people that don't know https://mui.com/system/sizing/#supported-values

Copy link
Member

Choose a reason for hiding this comment

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

I use it a lot to avoid prettier from breaking the lines. 😁

Copy link
Member Author

@oliviertassinari oliviertassinari Jun 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

haha, right, well, happy to hear other's perspectives on this. I think that the main value is the feature is so that developers can do:

Suggested change
height: '100%',
height: 1/2,

we supported height: 1 for consistency and to match styled-system. height: 2 generate a height of 2px 🙃. In MUI Core, we never use this API.

cc @mnajdova should we consider removing the documentation of it in https://mui.com/system/sizing/#supported-values?

Tailwind CSS used full.

Copy link
Member

Choose a reason for hiding this comment

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

cc @mnajdova should we consider removing the documentation of it in https://mui.com/system/sizing/#supported-values?

Agree, I remember another discussion some time ago about the same topic. Linking the comment to the v6 milestone for removal and yep, will remove the docs for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Idea moved to mui/material-ui#38583.

docs/data/data-grid/tree-data/TreeDataDisableChildrenSorting.tsx
@@ -124,7 +124,7 @@ export default function TreeDataDisableChildrenSorting() {
]);

return (
<Stack style={{ width: '100%' }} alignItems="flex-start" spacing={2}>
<Box sx={{ width: '100%' }}>
Copy link
Member Author

Choose a reason for hiding this comment

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

Less code

docs/data/data-grid/style/SxProp.js
@@ -10,7 +11,7 @@ export default function SxProp() {
});

return (
<div style={{ height: 300, width: '100%' }}>
<Box sx={{ height: 300, width: '100%' }}>
Copy link
Member Author

Choose a reason for hiding this comment

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

Avoid inline style per https://web.dev/csp/#inline-code-is-considered-harmful

docs/data/data-grid/state/RestoreStateApiRef.tsx
) => {
dispatch({ type: 'setNewViewLabel', label: e.target.value });
dispatch({ type: 'setNewViewLabel', label: event.target.value });
Copy link
Member Author

@oliviertassinari oliviertassinari Jun 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Avoid shorthands

docs/data/data-grid/state/RestoreStateApiRef.tsx
@@ -212,7 +212,7 @@ const NewViewListButton = (props: {
);
};

const CustomToolbar = () => {
function CustomToolbar() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Convention for top-level components

docs/data/data-grid/rows/rows.md Outdated Show resolved Hide resolved
docs/data/data-grid/export/export.md
Comment on lines -235 to -244
To install it:

```sh
// with npm
npm install exceljs

// with yarn
yarn add exceljs
```

Copy link
Member Author

Choose a reason for hiding this comment

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

No longer needed since we changed the dependencies in the package.json

docs/data/data-grid/export/export.md
@@ -206,7 +206,7 @@ For more option to customize the print export, please visit the [`printOptions`
:::warning
Due to the fact that the Print export relies on the usage of an `iframe`, there is a limitation around the usage of `X-Frame-Options`.

In order for the Print export to work as expected set `X-Frame-Options: SAMEORIGIN`.
In order for the Print export to work as expected set `X-Frame-Options: SAMEORIGIN` or unset the `X-Frame-Options` header.
Copy link
Member Author

Choose a reason for hiding this comment

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

Both works.

m4theushw
m4theushw approved these changes Jun 13, 2022
View reviewed changes
docs/data/data-grid/components/CustomFooter.tsx Outdated Show resolved Hide resolved
docs/data/data-grid/row-grouping/RowGroupingSortingSingleGroupingColDef.tsx Outdated Show resolved Hide resolved
docs/data/data-grid/rows/rows.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 14, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 14, 2022
@oliviertassinari oliviertassinari merged commit fd1028c into mui:master Jun 14, 2022
@oliviertassinari oliviertassinari deleted the docs-batch-small-changes branch June 14, 2022 23:53
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
@oliviertassinari @alexfauquette
[docs] Batch small changes (mui#5177)
67b0715
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnajdova mnajdova mnajdova left review comments

@m4theushw m4theushw m4theushw approved these changes

Assignees
No one assigned
Labels
docs Improvements or additions to the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oliviertassinari @mui-bot @mnajdova @m4theushw