kagux / ex_debug_toolbar

A debug web toolbar for Phoenix projects to display all sorts of information about request
https://hex.pm/packages/ex_debug_toolbar
407 stars 11 forks source link

Feature/toolbar ui poc #20

Closed kagux closed 7 years ago

kagux commented 7 years ago

image


This change is Reviewable

juanperi commented 7 years ago

Reviewed 22 of 22 files at r1. Review status: all files reviewed at latest revision, 2 unresolved discussions.


web/static/css/toolbar.scss, line 28 at r1 (raw file):

    .arrow {
      transform: translate(-15px, 0);
      -webkit-transform: translate(-15px, 0);

do you really need the prefixes???


web/static/js/toolbar.js, line 28 at r1 (raw file):

  renderToolbar({html: html, request: request}){
    console.log("Request: ", request);
    document.body.innerHTML += '<div class="ex-debug-toolbar">' + html + '</div>';

i'll swear that doing this forces a rerender of the whole page. It could lead to unexpected results. I can see 2 workarounds:

  1. We create a dom element, assign its content with innerHTML, and then append that element as the last child of body
  2. In the code injector plug, we create the div with the ex-debug-toolbar class (or id). Then, here we just replace the content of that div

Comments from Reviewable

kagux commented 7 years ago

Review status: 15 of 21 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


web/static/css/toolbar.scss, line 28 at r1 (raw file):

Previously, epilgrim (Juan Peri) wrote…
do you really need the prefixes???

I wasn't sure :) but you know what, it was supposed to shift popover from left border of screen, but it doesn't work anyway, I removed it. It works on live reload, because it updates css, but when js recalculates popover position, this css makes no difference :(


web/static/js/toolbar.js, line 28 at r1 (raw file):

Previously, epilgrim (Juan Peri) wrote…
i'll swear that doing this forces a rerender of the whole page. It could lead to unexpected results. I can see 2 workarounds: 1. We create a dom element, assign its content with innerHTML, and then append that element as the last child of body 2. In the code injector plug, we create the div with the ex-debug-toolbar class (or id). Then, here we just replace the content of that div

how about now?


Comments from Reviewable

juanperi commented 7 years ago

Awesome job!!! :lgtm_strong: :lgtm_strong: :lgtm_strong: :lgtm_strong:


Reviewed 6 of 6 files at r2. Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


Comments from Reviewable

kagux commented 7 years ago

thanks :)