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

Refactor ReportAbuseButton to use DismissibleTextForm #7382

Merged
merged 3 commits into from
Jan 18, 2019

Conversation

mirefly
Copy link
Contributor

@mirefly mirefly commented Jan 13, 2019

Fixes mozilla/addons#10834

Botton styles changes.
I also removed relevant part of disableAbuseButtonUI and enableAbuseButtonUI in "core/reducers/abuse.js" since they are not used any more.

Before,
image

After,
image

@codecov-io
Copy link

codecov-io commented Jan 13, 2019

Codecov Report

Merging #7382 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#7382      +/-   ##
==========================================
- Coverage   97.87%   97.86%   -0.01%     
==========================================
  Files         257      257              
  Lines        7044     7024      -20     
  Branches     1302     1299       -3     
==========================================
- Hits         6894     6874      -20     
  Misses        136      136              
  Partials       14       14
Impacted Files Coverage Δ
src/core/reducers/abuse.js 100% <ø> (ø) ⬆️
src/amo/components/ReportAbuseButton/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f29735d...c7eded3. Read the comment docs.

@willdurand willdurand self-assigned this Jan 14, 2019
Copy link
Member

@willdurand willdurand left a comment

Choose a reason for hiding this comment

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

Thanks, very good start!

src/amo/components/ReportAbuseButton/index.js Outdated Show resolved Hide resolved
// We need to use mount here because we're interacting with refs. (In
// this case, the textarea.)
const root = renderMount();
Copy link
Member

Choose a reason for hiding this comment

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

Is the comment above still accurate? We could maybe use shallow instead of mount here now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree the comment is not accurate any more. I tried shallow, but it seems that I need to call renderShallow again to make the expect line works. I am not sure why it does not rerender. If calling renderShallow twice is ok, I will change it to shallow.

let root = renderShallow({ addon, store });
root.find(Button).simulate('click', fakeEvent)
root = renderShallow({ addon, store });                                     
expect(root.find('.ReportAbuseButton--is-expanded')).toHaveLength(1);    

Copy link
Member

Choose a reason for hiding this comment

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

what if you call root.update() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither of root.update() / root.instance().forceUpdate() works. It looks like root (<ReportAbuseButtonBase/>) cannot get the updated props value from <ReportAbuseButton>. It might be related to enzymejs/enzyme#1229 (comment)

I found some other tests using render twice.

root = render({ collection, editing: true, store });

Copy link
Member

@willdurand willdurand Jan 17, 2019

Choose a reason for hiding this comment

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

I see. let's keep it as is then

tests/unit/amo/components/TestReportAbuseButton.js Outdated Show resolved Hide resolved
@willdurand
Copy link
Member

Hi @mirefly, it looks like you resolved all comments but you did not push any update yet.

@mirefly
Copy link
Contributor Author

mirefly commented Jan 17, 2019

@willdurand Thanks very much for help on this. I pushed the changes.

@mirefly mirefly force-pushed the refactor-report-abuse-form-3279 branch from e529f44 to adbd48f Compare January 17, 2019 17:59
@mirefly mirefly force-pushed the refactor-report-abuse-form-3279 branch from adbd48f to c7eded3 Compare January 17, 2019 18:06
Copy link
Member

@willdurand willdurand left a comment

Choose a reason for hiding this comment

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

r+

@willdurand willdurand merged commit 8319dad into mozilla:master Jan 18, 2019
@willdurand
Copy link
Member

Thanks @mirefly!

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.

Refactor report abuse form to use DismissibleTextForm
3 participants