[webkit-reviews] review granted: [Bug 179329] Web Inspector: Canvas Tab initial user experience is poor when no canvases exist : [Attachment 328407] Patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has granted Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 179329: Web Inspector: Canvas Tab initial user experience is poor when no
canvases exist
https://bugs.webkit.org/show_bug.cgi?id=179329

Attachment 328407: Patch

https://bugs.webkit.org/attachment.cgi?id=328407&action=review




--- Comment #7 from Timothy Hatcher <timothy at hatcher.name> ---
Comment on attachment 328407
  --> https://bugs.webkit.org/attachment.cgi?id=328407
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328407&action=review

> Source/WebInspectorUI/ChangeLog:13
> +	   This patch adds a new method, WI.createNavigationItemHelp, for
creating command
> +	   help to display in a message text view. The method accepts a format
string and
> +	   NavigationItem, and returns a DOM element. For example,
> +
> +	       WI.createNavigationItemHelp("Press %s to do X.", navigationItem)

Neat!

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:703
> +localizedStrings["Press %s to load a recording from file."] = "Press %s to
load a recording from file.";

This could be hard to localize because of the wording in the help button
replacement, but I'm not sure what would make it better.

> Source/WebInspectorUI/UserInterface/Base/Main.js:2271
> +    String.format(formatString, [wrapperElement], String.standardFormatters,
containerElement, append);

Clever.

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:39
> +	   let importNavigationItem = new
WI.ButtonNavigationItem("import-recording", WI.UIString("Import"),
"Images/Import.svg", 15, 15);
> +	   importNavigationItem.buttonStyle =
WI.ButtonNavigationItem.Style.ImageAndText;
> +	  
importNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Clicked, ()
=> { WI.canvasManager.importRecording(); });

Anyway to reuse the real ButtonNavigationItem from
RecordingNavigationSidebarPanel?

> Source/WebInspectorUI/UserInterface/Views/Main.css:204
> +.message-text-view .navigation-item-help .navigation-bar > .item {

Text size being the same larger size as the body of the message bothers me. I
kind of expect this text to exactly match the navigation bar text.