ember-inspector icon indicating copy to clipboard operation
ember-inspector copied to clipboard

add modifiers to render tree

Open patricklx opened this issue 1 year ago • 11 comments

Description

this adds modifiers and their html element to the render tree.

this also improves the UX for positional args, especially functions

this builds on #2549

Screenshots

patricklx avatar Feb 02 '24 15:02 patricklx

image

patricklx avatar Feb 06 '24 13:02 patricklx

this is ready. I tried to support all versions we currently check and also make it robust in case some things are not available. also, this is very close to the PR i have to glimmer-vm

patricklx avatar Feb 07 '24 13:02 patricklx

@patricklx the code and tests look great here, thank you for taking this on! I was looking at the screenshot though, and shouldn't the modifier be inside the element? They seem to be outside of the closing >.

RobbieTheWagner avatar Feb 08 '24 16:02 RobbieTheWagner

It's by design, i didn't try to put it inside. Maybe it can be done, i can have a look tomorrow

patricklx avatar Feb 08 '24 16:02 patricklx

I think it makes more sense to have it under the element for ux. Otherwise it can be too much on one line. But i could add a > after the last modifier of an element

patricklx avatar Feb 08 '24 17:02 patricklx

@RobbieTheWagner i tried a few things, but was not able to do it in a good way. It could have multiple positional args and named args as well. maybe add some indicator that its inside an element? Or a smaller indent. Or larger indent?

Screenshot 2024-02-09 at 14 22 33

patricklx avatar Feb 09 '24 13:02 patricklx

I really think it should look the same as what users see in their code.

<li class="foo" {{did-insert this.myCoolFunction}}>

I do understand these could be rather long, but I think we should figure out a way to wrap things. Like essentially we want this markup to look how prettier would format it.

RobbieTheWagner avatar Feb 09 '24 14:02 RobbieTheWagner

Wrapping is not good in this case, at least with CSS, since we are using vertical-collection which needs predictable/fixed row heights. I think . So it would need to be done manually. So everything would need to be in one line...

Also i often prefer to have it like this:

<li class="foo"
   {{did-insert this.myCoolFunction}}
>

:)

patricklx avatar Feb 09 '24 14:02 patricklx

how about this: Screenshot 2024-02-09 at 17 19 23

patricklx avatar Feb 09 '24 16:02 patricklx

how about this: Screenshot 2024-02-09 at 17 19 23

This looks great! 👍

RobbieTheWagner avatar Feb 09 '24 21:02 RobbieTheWagner

pushed the changes

patricklx avatar Feb 09 '24 23:02 patricklx

@RobbieTheWagner what about this?

patricklx avatar Feb 26 '24 16:02 patricklx

I found that this might need more work. The modifiers need to be removed from debug render tree when they get destroyed

patricklx avatar Feb 27 '24 11:02 patricklx

@RobbieTheWagner ready

patricklx avatar Feb 27 '24 16:02 patricklx

@patricklx looks like there are some conflicts. Would you mind resolving those please?

RobbieTheWagner avatar Mar 11 '24 23:03 RobbieTheWagner

@RobbieTheWagner rebased

patricklx avatar Mar 15 '24 21:03 patricklx