-
Notifications
You must be signed in to change notification settings - Fork 398
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
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. |
203ef1f
to
3f5e0aa
Compare
I updated this to stop using |
aa8ccea
to
79f859c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance to use Function
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmh. There must be something I missed here. I fail to see why this has been added 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
const createFakeChangeEvent = (value = '') => { | |
return createFakeEvent({ | |
target: { value }, | |
}); | |
}; |
wrapper.find('input').simulate('change', createFakeChangeEvent('& 26 %')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"is" sent, maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, sorry for the false positive then. I was thinking about sequence of tenses..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, it's confusing! 🤣
(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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+wc
This is looking good but I would like to see the button centered or, ideally, like the mock-ups (full-width) :)
Oh shoot, yes, I meant to say I would full-width it... I must've forgot, thanks for the reminder. Will do before merge! 👍 |
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