Report add-on abuse UI #3202
Report add-on abuse UI #3202
Conversation
Codecov Report
@@ Coverage Diff @@
## master mozilla/addons-frontend#3202 +/- ##
==========================================
+ Coverage 95.24% 95.92% +0.68%
==========================================
Files 158 159 +1
Lines 2924 3363 +439
Branches 577 707 +130
==========================================
+ Hits 2785 3226 +441
+ Misses 120 118 -2
Partials 19 19
Continue to review full report at Codecov.
|
|
Test failure is mozilla/addons#10799. |
tofumatt
force-pushed
the
report-abuse-ui-2759
branch
2 times, most recently
from
203ef1f
to
3f5e0aa
Compare
|
I updated this to stop using |
tofumatt
force-pushed
the
report-abuse-ui-2759
branch
2 times, most recently
from
aa8ccea
to
79f859c
Compare
There was a problem hiding this comment.
Looking good!
I don't really like lifting the UI state up to redux, but I can live with that.
-
I noticed a UI glitch that I specifically checked because you taught me to pay more attention to that a while ago ;-) The button does not have a click cursor on FF Nightly.
-
It is possible to send an empty review. I think we should
trim()the textarea value all the time, otherwise you can enter a space and validate. -
Can we horizontally center this button? I don't recall the mock-ups right now, but the stars in "rate your xp" are centered. This would be slightly more consistent IMO.
|
|
||
|
|
||
| type PropTypes = { | ||
| abuseReport: {| message: string, reporter: Object | null |}, |
There was a problem hiding this comment.
I find it generally more readable when properties have each their own line, and nested objects are indented.
There was a problem hiding this comment.
Me too, actually. Dunno what I was thinking 😉
| type PropTypes = { | ||
| abuseReport: {| message: string, reporter: Object | null |}, | ||
| addon: Object | null, | ||
| debounce: any, |
There was a problem hiding this comment.
Any chance to use Function instead?
There was a problem hiding this comment.
Oops, yes. Sometimes I start with any for everything and I forgot to tidy this up.
| abuseReport: {| message: string, reporter: Object | null |}, | ||
| addon: Object | null, | ||
| debounce: any, | ||
| dispatch: any, |
There was a problem hiding this comment.
Same here, I think Function is a bit better than any.
| addon: Object | null, | ||
| debounce: any, | ||
| dispatch: any, | ||
| errorHandler: any, |
There was a problem hiding this comment.
This should be an object no? Ideally, we should have a type for this since it is used in other files.
| const { addon, dispatch, loading } = this.props; | ||
|
|
||
| if (loading) { | ||
| return; |
There was a problem hiding this comment.
Either we should have a logger.debug or the button should be displayed at all when loading = true.
There was a problem hiding this comment.
The button content changes to indicate the report is being sent so I think it's useful to show (it's also less visually jarring).
But I'll add a debug statement, sure!
There was a problem hiding this comment.
Oh wait, this is a separate button. Agreed, actually, we can just hide this when loading.
Actually, no. It looks nicer to just disable it I think.
| type: HIDE_ADDON_ABUSE_REPORT_UI, | ||
| payload: { addon }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Other comments in this file also apply for the other new action creators.
| @@ -13,7 +64,7 @@ type LoadAddonAbuseReportType = { | |||
| }; | |||
|
|
|||
| export function loadAddonAbuseReport( | |||
| { addon, message, reporter }: LoadAddonAbuseReportType | |||
| { addon, message, reporter }: LoadAddonAbuseReportType = {} | |||
There was a problem hiding this comment.
Mmh. There must be something I missed here. I fail to see why this has been added 😢
There was a problem hiding this comment.
See above: I thought the errors were nicer. But if you think it's wasteful I'll remove them. I guess it's fine either way.
| // This simulates entering text into the textarea. | ||
| const textarea = root.find('.ReportAbuseButton-textarea textarea'); | ||
| textarea.node.value = 'add-on ate my homework!'; | ||
| textarea.simulate('change'); |
There was a problem hiding this comment.
Alternatively, you could pass a "change" event to simulate(), leveraging the createFakeEvent() helper:
addons-frontend/tests/unit/amo/components/TestSearchForm.js
Lines 86 to 90 in fa21471
There was a problem hiding this comment.
This wasn't working originally, but I forget why and maybe is not longer the case. It's definitely what I'd prefer–I'll try again.
There was a problem hiding this comment.
Oddly this won't work. Maybe because textarea is a weird DOM node or something, but this works so I'll just roll with it for now if that's alright.
| const root = renderMount({ addon, store }); | ||
|
|
||
| // Expand the view so we can test that it wasn't contracted when | ||
| // clicking on the disabled "dismiss" link. |
| expect(root.find('.ReportAbuseButton--is-expanded')).toHaveLength(1); | ||
| }); | ||
|
|
||
| it('shows a success message and hides the button if report was sent', () => { |
There was a problem hiding this comment.
English is annoying, but generally in conversation one would say "the letter was sent" not "the letter is sent". Similarly, one would say "The letter was not sent" instead of "the letter is not sent". Even with "yet", it'd be "the letter has not yet been sent" instead of "the letter is not sent yet".
Really this would be nicer if I wrote "if the report was sent" but I wanted to fit it on 80 chars. 😆
There was a problem hiding this comment.
Ah ok, sorry for the false positive then. I was thinking about sequence of tenses..
There was a problem hiding this comment.
No worries, it's confusing! 🤣
|
Gif for the cursor on FF Nightly:
|
|
(reducing @kumar303's PR review queue by unassigning him here) |
|
Thanks for the review! I'll tidy this up and request another one in a few. |
|
Changes made, ready for another r? |
|
|
|
Oh shoot, yes, I meant to say I would full-width it... I must've forgot, thanks for the reminder. Will do before merge! 👍 |
tofumatt
force-pushed
the
report-abuse-ui-2759
branch
from
67e8132
to
865800b
Compare
Fixes mozilla/addons#10566.
Allows a user to report an add-on for abuse. This uses a new inline form that I didn't bother to abstract for now, but it should be pretty easy to do if we use this UI elsewhere. It could be tweaked a bit but the UX is a lot nicer than a modal.
It logs reported add-ons in the reducer so once you've reported an add-on, at least for the current session (until a full page reload, because abuse reports are stored in a database/accessible via an API), it appears as reported and you can't submit another one.
Sorry for the big PR; it's mostly tests. I wanted to get the interaction quite nice and it's very tested, but it meant a lot of tests.
Desktop
Mobile