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

[Table] Migrate TableBody to emotion #24658

Closed
wants to merge 33 commits into from
Closed

Conversation

natac13
Copy link
Contributor

@natac13 natac13 commented Jan 27, 2021

#24405

@mui-pr-bot
Copy link

mui-pr-bot commented Jan 27, 2021

@material-ui/core: parsed: +0.08% , gzip: +0.06%

Details of bundle changes

Generated by 🚫 dangerJS against 480c2f8

@oliviertassinari oliviertassinari added the component: table This is the name of the generic UI component, not the React module! label Jan 27, 2021
@mnajdova
Copy link
Member

For fixing the error - https://app.circleci.com/pipelines/github/mui-org/material-ui/37282/workflows/f4054c2a-44af-4550-8ada-1e1c75514d81/jobs/220749

I believe we need to introduce a new option to the describeConformanceV5, so that we can wrap the element in <table /> and the structure would be correct. We can use something similar to the mount option, or introduce a callback of how to find the element, for example node.firstChild.firstChild and invoke the describeConformanceV5 with describeConformance(renderInTable(<TableBody />), ....).

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 28, 2021
@natac13
Copy link
Contributor Author

natac13 commented Jan 28, 2021

Alright I have taken a shot at getting conformanceV5 working with a wrapper function.
I do not think the way I have done this is the best and I am very open to suggestions.
I am still failing the theme variant props test.

@mnajdova
Copy link
Member

Alright I have taken a shot at getting conformanceV5 working with a wrapper function.
I do not think the way I have done this is the best and I am very open to suggestions.
I am still failing the theme variant props test.

Did you try to introduce some render option, similar to how the mount is defined? I think that would require less changes in the tests themselves. If the option is not provided, we can default to the render we are using at this moment. cc @oliviertassinari for thoughts on this

@natac13
Copy link
Contributor Author

natac13 commented Jan 28, 2021

Oh I see what you mean.
Provide a render to wrap in table but use the normal render when not.
I'll make the changes

@mnajdova
Copy link
Member

Oh I see what you mean.
Provide a render to wrap in table but use the normal render when not.
I'll make the changes

Exactlly, just watch out that the return result from render is a bit different, I suppose you would need to alter the container from the result object.

@natac13
Copy link
Contributor Author

natac13 commented Jan 28, 2021

Ok let me know what you think of 370b4c5

Maybe once we get this figured out I should wait till it gets merged to next, then rebase next for the TableCell PR?

@mnajdova
Copy link
Member

Ok let me know what you think of 370b4c5

Maybe once we get this figured out I should wait till it gets merged to next, then rebase next for the TableCell PR?

Exactly what I had in mind, I would maybe just suggest renaming the param to render, but let's wait on @oliviertassinari or @eps1lon feedback before doing any other changes. Also would propose moving these changes in a separate PR

@natac13
Copy link
Contributor Author

natac13 commented Jan 28, 2021

Also would propose moving these changes in a separate PR

I was just wondering the same thing as I re-read my comment

Ill wait to hear the instructions.

Ill Start on a none table component until we get this merged into next

classes,
inheritComponent: 'tbody',
mount: (node) => {
const wrapper = mount(<table>{node}</table>);
return wrapper.find('table').childAt(0);
},

wrapRender: (node) => {
Copy link
Member

@oliviertassinari oliviertassinari Jan 28, 2021

Choose a reason for hiding this comment

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

Agree with Marija, render sounds better and more consistent with what Sebastian did with mount.

Suggested change
wrapRender: (node) => {
render: (node) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Am I still ok to use wrapRender in the conformance tests as the name would conflict with the normal render?

classes,
inheritComponent: 'tbody',
mount: (node) => {
const wrapper = mount(<table>{node}</table>);
return wrapper.find('table').childAt(0);
},

wrapRender: (node) => {
Copy link
Member

Choose a reason for hiding this comment

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

Marking this for future follow-up: check-out something like render(node, { container: document.createElement('tabl') }) (need to check for connectedness and cleanup)

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 28, 2021
natac13 and others added 22 commits January 30, 2021 09:41
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@natac13
Copy link
Contributor Author

natac13 commented Jan 30, 2021

I suck at rebasing... Ill open a new PR

@natac13 natac13 closed this Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: table This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.