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] Align codebase a bit with conventions #55

Merged
merged 5 commits into from
Jul 11, 2020
Merged

[docs] Align codebase a bit with conventions #55

merged 5 commits into from
Jul 11, 2020

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jul 8, 2020 •
edited
Loading

A chunk of #6

@oliviertassinari oliviertassinari requested a review from dtassone July 8, 2020 17:36
@oliviertassinari oliviertassinari changed the title [docs] Align codebase with conventions [docs] Align codebase a bit with conventions Jul 8, 2020
oliviertassinari
oliviertassinari commented Jul 8, 2020
View reviewed changes
packages/storybook/src/stories/playground/customize-components.stories.tsx
return (
<div className="grid-container">
<XGrid
rows={rows}
columns={columns}
components={{ loadingOverlay: loadingComponent }}
components={{ loadingOverlay: LoadingComponent }}
Copy link
Member Author

@oliviertassinari oliviertassinari Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This makes me wonder, shouldn't we rename the key to a capital first letter to

  1. Communicate it's a component
  2. Allow the shorthand notation

?

Suggested change
components={{ loadingOverlay: LoadingComponent }}
components={{ LoadingOverlay }}

Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually what 2 of the 3 prior arts I could find on mui/material-ui#21453 do.

Copy link
Member

Choose a reason for hiding this comment

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

Yes we could as we replace our internal components with the user components

oliviertassinari
oliviertassinari commented Jul 8, 2020
View reviewed changes
packages/storybook/src/stories/playground/customize-components.stories.tsx
export const Loading = () => {
const loadingComponent = () => (
<GridOverlay className={'custom-overlay'}>
function LoadingComponent() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
function LoadingComponent() {
function LoadingOverlay() {

Copy link
Member

Choose a reason for hiding this comment

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

If we take this approach we have to do it for all the components :)

Copy link
Member Author

@oliviertassinari oliviertassinari Jul 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't mean to apply the change here. This was meant for illustration purposes on the RFC :). I think that we should wait for the conclusion of the RFC, but yeah, then we enforce it everywhere.

@oliviertassinari oliviertassinari force-pushed the demo-consistency-with-main branch from 2137834 to 6f5eab4 Compare July 9, 2020 22:37
@oliviertassinari oliviertassinari force-pushed the demo-consistency-with-main branch from 6f5eab4 to 346a1f6 Compare July 9, 2020 22:41
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 9, 2020 •
edited
Loading

@dtassone I have rebased the pull request to fix the conflicts. Please have a second look. A couple of problems I have noticed that I couldn't fix here:

  1. Demo isolation. We should get one file per demo. We shouldn't depend on external styles (CSS), people should be able to copy and paste the whole source of the demo and have it working in their end. I can't wait for we migrate these demos to their final format :).
  2. The --report-unused-disable-directives rule of eslint doesn't work. I don't know why. There is a bunch of disabled eslint warnings that has no effect. Same writing none sene in the demo doesn't report any issue. Something is not right.

@oliviertassinari
Copy link
Member Author

  1. Found it: [core] Fix eslint #60

@oliviertassinari oliviertassinari merged commit 88eede3 into master Jul 11, 2020
@oliviertassinari oliviertassinari deleted the demo-consistency-with-main branch July 11, 2020 16:35
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation component: data grid This is the name of the generic UI component, not the React module! labels Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtassone dtassone Awaiting requested review from dtassone

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@oliviertassinari @dtassone