Add args to tree

[RobbieTheWagner]

Rebasing seemed hard, so opening a new PR

[RobbieTheWagner]

@chancancode @nummi there were a lot of conflicts since the big PR was merged, so I cherry picked our commits into this new PR. Please take a look!

[chancancode]

I left some review comments, but I think the main issue is still the horizontal scrolling CSS issues? (scrolling breaks the row highlight/closing tag/buttons)

See https://discordapp.com/channels/480462759797063690/486243207072710656/654839741384228888

[nummi]

I left some review comments, but I think the main issue is still the horizontal scrolling CSS issues?

I don't know if we'll fix this before the next release. It has been an issue for a while and I'm afraid the fix will require some big changes to the markup and/or the CSS.

[chancancode]

@nummi I don't think the problem is specific to our setup here, you can reproduce it with a pretty minimal html/css file:

<style>
.scrollable {
  overflow: scroll;
  width: 100%;
  height: 100%;
}

.row {
  background-color: lightgrey;
  height: 50px;
}

.row:nth-child(2n) {
  background-color: lightslategrey;
}
</style>

<div class="scrollable">
  <div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div>

  <div class="row" style="width: 5000px"></div>

  <div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div><div class="row"></div>
</div>

I think we may have to switch to flexbox, or perhaps there is some other CSS Trick™ that would make it work here, but I'm pretty sure we can make something work. Since it's a pretty major new feature, it's okay for it to take a bit longer (the 3.13.0 release already went out yesterday, so it's not a blocker), but we should find a way to introduce it without introducing a regression.

[RobbieTheWagner]

@chancancode I was talking to @nummi earlier and I think we should move the action icons into the object inspector. That would solve the issue of them being off the screen.

[RobbieTheWagner]

@chancancode I believe I resolved all the things you requested. Mind taking another look? I think we should get this in, and then have @nummi help us move the icons to the object inspector, so people do not have to scroll to see them.

[nummi]

[nummi]

[RobbieTheWagner]

Nice work @nummi!

[nummi]

#1129

[nummi]

Looks good from my end. UI works well on light and dark modes; scrolling issue is fixed; args in Glimmer components and no args on classic.

Changes were made

[chancancode]

Looks pretty good! A few more things to fix then I think we are golden

[chancancode]

why is this removed?

[nummi]

As far as I could tell that was being used for the hover state. See: https://github.com/emberjs/ember-inspector/pull/1129/files#diff-cbd2aa0d940eef1f44430e6ca030fbd9L8

[nummi]

Here is the original PR: #984

[chancancode]

I see. Yeah that changed in Octane. It is now used to highlight the "currently previewing component". IMO the previous behavior is not that useful, since you can already see the parent/child relationship from the nesting.

There are two ways to "preview" a component, by hovering the rows in the component tree or by using the "live inspection" feature and hover over the DOM elements on the page:

Using CSS hover state only solves the first part and I think we still need this for the latter.

[nummi]

Oooooh. I see. I'll have to revert that.

[chancancode]

args in Glimmer components and no args on classic

@nummi are you testing both on the same app? I think we expect args to show up for both, assuming you have a recent enough version of ember-source. So if it's not showing up for classic components, there may be a bug.

[nummi]

@nummi are you testing both on the same app? I think we expect args to show up for both, assuming you have a recent enough version of ember-source. So if it's not showing up for classic components, there may be a bug.

I am not. The classic app I tested in is deprecations.emberjs.com and the Glimmer app is ember-source: 3.15.0

[chancancode]

@nummi 👌 that's fine then. if you have any classic components in the 3.15 app, they would/should also show up with args.

Another thing I noticed is sometimes the strings passed to the components can be very long (like an entire blog post). Do we want to come up with a reasonable limit and truncate the rest with ...? (The component-argument can handle this pretty naturally.)

[RobbieTheWagner]

@chancancode coming up with a limit sounds good.

[nummi]

#1135

[chancancode]

@chancancode coming up with a limit sounds good.

Feel free to steal this:

function truncate(str, limit = 20, rest = "...") {
  if (str.length <= limit) {
    return str;
  }

  if (rest.length >= limit) {
    throw new Error("rest cannot be longer than limit");
  }

  let parts = str.split(/(\s+|\b)/); // split by whitespace or word boundaries
  let selected = []; // the parts to keep
  let targetLength = limit - rest.length; // leave room for the "..."
  let currentLength = 0;

  while (true) {
    let candidate = parts.shift();

    if ((currentLength + candidate.length) <= targetLength) {
      selected.push(candidate);
      currentLength += candidate.length;
    } else {
      if (currentLength === 0) {
        // We have no choice but to break in the middle of a long word
        selected.push(candidate.slice(0, targetLength));
      }

      break;
    }
  }

  selected.push(rest);

  return selected.join("")
}

It ensures the resulting string (including the "..." if added) is shorter than the given limit, but try to break at word boundaries or whitespaces when possible:

> truncate("It ensures the resulting string (including the \"...\" if added) is shorter than the given limit, but try to break at word boundaries or whitespaces when possible:", 999)
"It ensures the resulting string (including the "..." if added) is shorter than the given limit, but try to break at word boundaries or whitespaces when possible:"

> truncate("It ensures the resulting string (including the \"...\" if added) is shorter than the given limit, but try to break at word boundaries or whitespaces when possible:", 100)
"It ensures the resulting string (including the "..." if added) is shorter than the given limit, ..."

> truncate("It ensures the resulting string (including the \"...\" if added) is shorter than the given limit, but try to break at word boundaries or whitespaces when possible:", 50)
"It ensures the resulting string (including the ..."

> truncate("It ensures the resulting string (including the \"...\" if added) is shorter than the given limit, but try to break at word boundaries or whitespaces when possible:", 20)
"It ensures the ..."

> truncate("It ensures the resulting string (including the \"...\" if added) is shorter than the given limit, but try to break at word boundaries or whitespaces when possible:", 10)
"It ..."

With this style the truncation, I find that 20 seems to be a pretty reasonable default (it will often be much shorter than that depending the length of the word that happen to sit at the boundary).

[nummi]

Any chance we could use an ellipsis in place of three periods? "…"

With a monospaced font:
three periods: ...
ellipsis: …

[chancancode]

Sounds good to me. The algorithm should work just as well

[RobbieTheWagner]

@chancancode I just pushed up the truncate and the adding quotes you requested. I was unable to test out the positional params because it seems like the renderNode is coming back with named args only, not positional ones, even when I use {{set-body-class this.theme}}