1307919 - Add learn more link

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Lin Clark [:linclark]]

Originally posted by:linclark

see https://github.com/devtools-html/gecko-dev/issues/229

Currently in the web console we show learn more links. we need to add these to things like mixed content warnings.

Manual testing page:
- https://googlesamples.github.io/web-fundamentals/samples/fundamentals/security/prevent-mixed-content/image-gallery-example.html

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1307919 - Add "Learn more" MDN link in message output. ;

Review commit: https://reviewboard.mozilla.org/r/86298/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86298/

Comment 2

[Lin Clark [:linclark]]

Comment on attachment 8801688 [details]
Bug 1307919 - Add "Learn more" MDN link in message output. ;

https://reviewboard.mozilla.org/r/86298/#review85252

I haven't had a chance to manually test this yet (waiting on a build), but overall it looks good to me. Just a few comments below. I'm marking as r- because I think there will be a warning about keys, but that's easy to fix.

::: devtools/client/webconsole/new-console-output/components/message-types/console-command.js:38
(Diff revision 1)
>    const {
>      source,
>      type,
>      level,
>      messageText: messageBody,
> +    exceptionDocURL,

Will console commands ever have an exception doc URL?

::: devtools/client/webconsole/new-console-output/components/message.js:165
(Diff revision 1)
>        icon,
>        collapse,
>        dom.span({ className: "message-body-wrapper" },
>          dom.span({ className: "message-flex-body" },
>            dom.span({ className: "message-body devtools-monospace" },
> -            messageBody
> +            [messageBody, learnMore]

You shouldn't need to make this an array. If you make this an array, it likely sets off React's warning about keys. Instead, you can just add learnMore as an additional parameter (as repeat and location are for the parent span).

::: devtools/client/webconsole/new-console-output/test/components/evaluation-result.test.js:48
(Diff revision 1)
>      expect(wrapper.find(".message-body").text())
> -      .toBe("ReferenceError: asdf is not defined");
> +      .toBe("ReferenceError: asdf is not defined[Learn More]");
>  
>      expect(wrapper.find(".message.error").length).toBe(1);
> +
> +    // There should be a [Learn more] link.

Maybe we should break this out into a separate it() statement?

::: devtools/client/webconsole/new-console-output/test/components/page-error.test.js:51
(Diff revision 1)
>      const locationLink = wrapper.find(`.message-location`);
>      expect(locationLink.length).toBe(1);
>      // @TODO Will likely change. See https://github.com/devtools-html/gecko-dev/issues/285
>      expect(locationLink.text()).toBe("test-tempfile.js:3:5");
> +
> +    // There should be a [Learn more] link.

Ditto.

Comment 3

[Lin Clark [:linclark]]

I just manually tested and it looks good to me. With the fixes, this will be an r+.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8801688 [details]
Bug 1307919 - Add "Learn more" MDN link in message output. ;

https://reviewboard.mozilla.org/r/86298/#review85252

> Will console commands ever have an exception doc URL?

No you're right. Forgot to clean this one

> Maybe we should break this out into a separate it() statement?

yes, it makes sense

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8801688 [details]
Bug 1307919 - Add "Learn more" MDN link in message output. ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86298/diff/1-2/

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

TRY looks fine https://treeherder.mozilla.org/#/jobs?repo=try&revision=bab07a6334d8

Comment 7

[Lin Clark [:linclark]]

Comment on attachment 8801688 [details]
Bug 1307919 - Add "Learn more" MDN link in message output. ;

https://reviewboard.mozilla.org/r/86298/#review85716

Awesome! Bit by bit we're getting closer to dev edition. Thanks for your work on this.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2511b7cd8d9
Add "Learn more" MDN link in message output. r=linclark;

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e2511b7cd8d9