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

[List, Edit, Create, Show] Wrap view components into styles HOC instead of wrapping main components #3161

Merged
merged 6 commits into from
Apr 25, 2019

Conversation

cherniavskii
Copy link
Contributor

When using ShowController and ShowView instead of single Show component, Show styles aren't used anymore.
This makes aside component appear under show view (when using Show component, aside is on the right).

Wrapping ShowView with styles HOC resolves the issue. Also, it makes more sense, since styles are related to view component, not Show component, which is simply composition of controller and view.

@fzaninotto
Copy link
Member

You're totally right, it makes more sense. Would you mind doing the same for Edit and List, so that all views are consistent?

@@ -193,4 +193,4 @@ Show.propTypes = {
title: PropTypes.any,
};

export default withStyles(styles)(Show);
export default Show;
Copy link
Member

Choose a reason for hiding this comment

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

It does not make much sense to have both a named and a default export for the same object. Remove the named export (you will have to change the import in Show.spec.js, too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@cherniavskii
Copy link
Contributor Author

Did the same for Edit, Create, and List components.
I have troubles with ListView unit tests though. It uses enzyme shallow rendering, e.g. for checking that Card component is rendered.

@cherniavskii cherniavskii changed the title [Show] Wrap ShowView into styles HOC instead of wrapping Show component [List, Edit, Create, Show] Wrap view components into styles HOC instead of wrapping main components Apr 25, 2019
@cherniavskii
Copy link
Contributor Author

cherniavskii commented Apr 25, 2019

@fzaninotto I've managed to fix that by using wrapper.dive().

Generally, I'm not a big fan of enzyme unit tests. They are tightly coupled with implementation details, which leads to false positives like this one.

@fzaninotto fzaninotto merged commit c0fbd06 into marmelab:master Apr 25, 2019
@fzaninotto
Copy link
Member

Awesome, thanks!

@fzaninotto fzaninotto added this to the 2.9.0 milestone Apr 25, 2019
@cherniavskii cherniavskii deleted the ShowView-styles branch April 25, 2019 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants