-
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
Expose whether a version is soft or hard-blocked in the blocked page #13285
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13285 +/- ##
=======================================
Coverage 98.28% 98.28%
=======================================
Files 267 267
Lines 10612 10615 +3
Branches 3228 3236 +8
=======================================
+ Hits 10430 10433 +3
Misses 169 169
Partials 13 13 ☔ View full report in Codecov by Sentry. |
block?.soft_blocked.length && | ||
(block?.soft_blocked.includes(match.params.versionId) || | ||
!block?.blocked.length); | ||
let title; |
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.
Should we expose versionId
somewhere in the title or copy? (To me, it's expected that if we're rendering the content based on the versionId, we would mention that versionId somewhere on the page).
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.
We specifically dropped the "Versions blocked" text partly because it could be confusing, I fear this could have similar issues: if you show the version number alone, it doesn't say anything about the other versions, and that could be misleading ? I've asked in the Add-ons UX slack channel though, and I'll file and implement a follow-up if we decide we want the version in the title/headline.
i18n.gettext(`This add-on violates %(startLink)sMozilla's | ||
Add-on Policies%(endLink)s.`), | ||
i18n.gettext(`This extension, theme, or plugin violates | ||
%(startLink)sMozilla's Add-on Policies%(endLink)s.`), |
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.
%(startLink)sMozilla's Add-on Policies%(endLink)s.`), | |
%(startLink)sMozilla's add-on policies%(endLink)s.`), |
}, | ||
) | ||
: i18n.gettext( | ||
`This add-on is blocked for violating Mozilla policies.`, |
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 title doesn't appear to have a period at the end in the spec.
const isSoftBlocked = | ||
block?.soft_blocked.length && | ||
(block?.soft_blocked.includes(match.params.versionId) || | ||
!block?.blocked.length); |
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 a selector (e.g. isSoftBlocked(guid, versionId)
defined in the reducer file) or at least computed in mapStateToProps
IMO.
Fixes mozilla/addons#15016