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

[RFR]: use makeStyles in Field Components #3562

Merged
merged 7 commits into from
Aug 22, 2019
Merged

[RFR]: use makeStyles in Field Components #3562

merged 7 commits into from
Aug 22, 2019

Conversation

Kunnu01
Copy link
Contributor

@Kunnu01 Kunnu01 commented Aug 21, 2019 •
edited
Loading

ref: #3453
Converts following components:

  • BooleanField
  • ChipField
  • FileField
  • ImageField

@Kunnu01
Copy link
Contributor Author

Kunnu01 commented Aug 21, 2019 •
edited
Loading

@djhi @fzaninotto
In FileField test cases, some tests are failing when comparing the classes of the component.
For eg - here
expected is <div class=" "></div> and output is <div class="makeStyles-root-1" classes="[object Object]"></div> because of makeStyles.
Similar is the issue with ImageField test cases.
What should I do in this case?
Should I just hardcode the expected result accordingly?

@djhi
Copy link
Collaborator

djhi commented Aug 21, 2019 •
edited
Loading

Would be better to migrate these tests to testing-library instead of enzyme imo

@Kunnu01 Kunnu01 changed the title [WIP]: use makeStyles in Field Components [RFR]: use makeStyles in Field Components Aug 22, 2019
@Kunnu01
Copy link
Contributor Author

Kunnu01 commented Aug 22, 2019

Done, please review.

djhi
djhi reviewed Aug 22, 2019
View reviewed changes
Copy link
Collaborator

@djhi djhi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Would you mind migrating all the related tests to testing-library ?

@djhi
Copy link
Collaborator

djhi commented Aug 22, 2019

Sorry, I updated my comment after posting it. I meant all the tests for those files, not just the ImageField.

@Kunnu01 Kunnu01 changed the title [RFR]: use makeStyles in Field Components [WIP]: use makeStyles in Field Components Aug 22, 2019
@Kunnu01
add makeStyles in BooleanField
75dcf88 
ref: marmelab#3453

use useStyles in ChipField

use useStyles in FileField

use useStyles in ImageField

fix FileField's tests

fix tests

migrate ImageField tests to testing-library

fix tests

migrate tests of ChipField and FileField
djhi
djhi requested changes Aug 22, 2019
View reviewed changes
Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Awesome, almost there!

@Kunnu01 Kunnu01 changed the title [WIP]: use makeStyles in Field Components [RFR]: use makeStyles in Field Components Aug 22, 2019
djhi
djhi approved these changes Aug 22, 2019
View reviewed changes
Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

💪

@djhi djhi added this to the 3.0.0 milestone Aug 22, 2019
@djhi djhi merged commit 447ae2f into marmelab:next Aug 22, 2019
@fzaninotto
Copy link
Member

Sorry, but I had to force push to next to avoid a complicated rebase. As a consequence, the commits from this PR are no longer in next. Would you mind opening a new PR with the same commits?

@fzaninotto fzaninotto removed this from the 3.0.0 milestone Aug 22, 2019
@djhi
Copy link
Collaborator

djhi commented Aug 22, 2019

As this is my fault (I shouldn't have merged this PR), I'll take care of it @Kunnu01

@djhi djhi mentioned this pull request Aug 22, 2019
Merged
@fzaninotto fzaninotto added this to the 3.0.0 milestone Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djhi djhi djhi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Kunnu01 @djhi @fzaninotto