Closed Bug 1429296 Opened 2 years ago Closed 2 years ago

Show MDN link of filter properties when hover the filterbox

Categories

(DevTools :: Netmonitor, enhancement, P2)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: gasolin, Assigned: Honza)

References

()

Details

Attachments

(2 files)

follow design in https://mozilla.invisionapp.com/share/2XEEY0RYA#/screens/263401584_Network_Toolbar

Show MDN link of filter properties when user hover the filterbox
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee: gasolin → nobody
Status: ASSIGNED → NEW
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Comment on attachment 8971553 [details]
Bug 1429296 - Show MDN link of filter properties when hover the filterbox;

https://reviewboard.mozilla.org/r/240308/#review246232

Added a few comments, none of which should be blockers.

::: devtools/client/shared/components/MdnLink.js:18
(Diff revision 1)
> -
> -function MDNLink({ url }) {
>    return (
>      a({
>        className: "devtools-button learn-more-link",
> -      title: LEARN_MORE,
> +      title: title,

Do we need the ":title"?  Can it just be `title,`?

::: devtools/client/netmonitor/src/components/Toolbar.js:39
(Diff revision 1)
>  // Localization
>  const SEARCH_KEY_SHORTCUT = L10N.getStr("netmonitor.toolbar.filterFreetext.key");
>  const SEARCH_PLACE_HOLDER = L10N.getStr("netmonitor.toolbar.filterFreetext.label");
>  const TOOLBAR_CLEAR = L10N.getStr("netmonitor.toolbar.clear");
>  const TOOLBAR_TOGGLE_RECORDING = L10N.getStr("netmonitor.toolbar.toggleRecording");
> +const LEARN_MORE_TITLE = L10N.getStr("netmonitor.headers.learnMore");

"Learn more" doesn't seem very informative -- learn more about what?

Canw e make this message more descriptive, like "Learn more about Net Monitor"?
Attachment #8971553 - Flags: review?(dwalsh) → review+
Attached image Dark.gif
The mockups show the (?) as light grey, but the patch makes the (?) fairly dark, almost darker than every other bit of text (could just be the SVG width).  Should we lighten the (?) or is that a side effect of the mockups?
Flags: needinfo?(odvarko)
Thanks for the review David!

(In reply to David Walsh :davidwalsh from comment #2)
> >        className: "devtools-button learn-more-link",
> > -      title: LEARN_MORE,
> > +      title: title,
> 
> Do we need the ":title"?  Can it just be `title,`?
Fixed

> > +const LEARN_MORE_TITLE = L10N.getStr("netmonitor.headers.learnMore");
> 
> "Learn more" doesn't seem very informative -- learn more about what?
True, changed to: "Learn more about filtering"

> The mockups show the (?) as light grey, but the patch makes the (?) fairly
> dark, almost darker than every other bit of text (could just be the SVG
> width).  Should we lighten the (?) or is that a side effect of the mockups?
Good call

@Victoria: the MDN icon is grey on the mockup, but the existing MDN icons are dark (it's dark in Headers side panel when hovering over it and also dark in Statistics view). What color is the right one?

Honza
Flags: needinfo?(odvarko)
Flags: needinfo?(victoria)
(In reply to Jan Honza Odvarko [:Honza] from comment #4)

> @Victoria: the MDN icon is grey on the mockup, but the existing MDN icons
> are dark (it's dark in Headers side panel when hovering over it and also
> dark in Statistics view). What color is the right one?

I think that since the MDN icon is a secondary action that's inside an input field, it should be Gray-50 (like the icons in the main Firefox input bar)

(I think this makes sense when this toolbar is white - I'll file a separate bug for tweaks)
Flags: needinfo?(victoria)
(In reply to Victoria Wang [:victoria] from comment #6)
> I think that since the MDN icon is a secondary action that's inside an input
> field, it should be Gray-50 (like the icons in the main Firefox input bar)
I see, fixed.

Honza
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42347190c35a
Show MDN link of filter properties when hover the filterbox; r=davidwalsh
https://hg.mozilla.org/mozilla-central/rev/42347190c35a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
QA Whiteboard: [good first verify]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.