[webkit-reviews] review granted: [Bug 124363] Web Inspector: "data detectors" menu on hover for actionable tokens : [Attachment 218448] Patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Antoine Quint
<graouts at apple.com>'s request for review:
Bug 124363: Web Inspector: "data detectors" menu on hover for actionable tokens
https://bugs.webkit.org/show_bug.cgi?id=124363

Attachment 218448: Patch
https://bugs.webkit.org/attachment.cgi?id=218448&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218448&action=review


> Source/WebInspectorUI/UserInterface/HoverMenu.css:47
> +    -webkit-transform: translate3d(-3px, -5px, 0);

Does this need to be 3D? Just translate() should work. Maybe I am old school,
but negative margin-left and margin-right would work too.

> Source/WebInspectorUI/UserInterface/HoverMenu.css:62
> +    -webkit-transform: translateX(14px);

I'd use margin for this.

> Source/WebInspectorUI/UserInterface/HoverMenu.js:46
> +    constructor: WebInspector.HoverMenu,
> +
> +    __proto__: WebInspector.Object.prototype,

Now blank line.

> Source/WebInspectorUI/UserInterface/HoverMenu.js:67
> +	   setTimeout(function() {
> +	       element.classList.add(WebInspector.HoverMenu.VisibleClassName);
> +	   });

Should explicitly say 0 here. I've also been wanting to use one-off
requestAnimationFrame to sync better with the display for things like this.

requestAnimationFrame(function() {
element.classList.add(WebInspector.HoverMenu.VisibleClassName); });

> Source/WebInspectorUI/UserInterface/HoverMenu.js:69
> +	   window.addEventListener("scroll", this, true);

Do scroll events in overflows bubble? I'm not sure if this will catch any
scrolling.