feat: Show compatibility errors in Disco Pane (fix #2095) #2097
Conversation
|
@tofumatt - this looks to have bit-rotted, can it be updated and completed? |
|
|
tofumatt
force-pushed
the
show-disabled-state-on-disco-pane-2095
branch
from
3ebf6c2 to
ca3f6fc
Compare
tofumatt
changed the title
tofumatt
force-pushed
the
show-disabled-state-on-disco-pane-2095
branch
from
bf3ea0f to
a86c26e
Compare
|
Alright, updated and fixed everything, here are some screenshots: Small screen
Desktop
|
|
This is probably a @pwalm question but I wonder if the red could be dropped back a bit? The sea of red for a non-FF browser might be a bit much. |
There was a problem hiding this comment.
r+wc
Looks good, I think the main issue here is tidying up the findDOMNode usage. FWIW we don't currently use the lint rule because the historical usage needs to be cleaned up. See https://github.com/mozilla/addons-frontend/issues/968
|
|
||
| &, | ||
| a, | ||
| a:link { |
There was a problem hiding this comment.
Any other states needed e.g: visited, hover, focus, active etc or are the defaults ok?
There was a problem hiding this comment.
The defaults are fine, this just keeps the colour white which it will stay for those other states too.
| i18n.gettext('from %(authorName)s, %(timestamp)s'), | ||
| { authorName: review.userName, timestamp }); | ||
|
|
||
| const reviewBodySanitized = sanitizeHTML(nl2br(review.body), ['br']); |
There was a problem hiding this comment.
Is this intermediate const needed?
There was a problem hiding this comment.
It tidies up the next lines and makes it a bit more readable. I went back and forth on it and this was the nicest version.
| throw new Error('minVersion is required; it cannot be undefined'); | ||
| } | ||
|
|
||
| if (reason === INCOMPATIBLE_NOT_FIREFOX) { |
There was a problem hiding this comment.
Should never really happen since non non-FF browsers should ever see the disco pane.
There was a problem hiding this comment.
True, but they could hit the URL and it doesn't hurt to have nice messages. Could also happen if the UA was weird and might be useful in a bug report...
That said it increases code size. If you want, I can remove this if we care a lot less about other clients in the Disco Pane, which I guess we do.
|
|
||
| expect(preventDefault.called).toBeTruthy(); | ||
| expect(installTheme.calledWith(themeImage, data.addon)).toBeTruthy(); | ||
| sinon.assert.calledWith(installTheme, themeImage); |
There was a problem hiding this comment.
is data.addon no longer passed?
There was a problem hiding this comment.
This was actually a case of "how did this test ever work?" since updating the withInstallButton HOC. I fixed it up using enzyme... but we should definitely clean up this test suite in general...
| const root = renderAddon({ addon: result, ...result }); | ||
| expect(root.heading.textContent).toContain('test-heading'); | ||
|
|
||
| expect(findDOMNode(root).querySelector('.heading').textContent) |
There was a problem hiding this comment.
Iirc findDOMNode was planned to be deprecated see https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-find-dom-node.md
See also https://facebook.github.io/react/docs/react-dom.html#finddomnode re: use being discouraged.
There was a problem hiding this comment.
It is, and we'll be moving to enzyme, but I didn't want to refactor these tests at the time. I could do it now if you like but this isn't our only usage of findDomNode 😄
(The refs weren't working, so I moved to the DOM queries.)
|
|
||
| it('renders a notice for old versions of Firefox', () => { | ||
| const root = render({ | ||
| lang: 'fr', |
There was a problem hiding this comment.
Seems a little odd to pass lang and then be looking for english translations.
There was a problem hiding this comment.
Oh that's true, I'll tweak that.
tofumatt
force-pushed
the
show-disabled-state-on-disco-pane-2095
branch
from
a86c26e to
1771519
Compare
|
The red is a bit much, isn't it. I would suggest we put that "You need to..." notification between the intro copy and the first add-on, and remove or make inactive the "Add to Firefox"/"Install Theme" buttons. |
Working on showing errors in the disco pane; tests are failing right now and it's low-priority so just leaving here now but will revisit later.