Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resizable debugger sidebar #46

Closed
wants to merge 3 commits into from
Closed

Resizable debugger sidebar #46

wants to merge 3 commits into from

Conversation

rtfeldman
Copy link
Member

@rtfeldman rtfeldman commented Nov 4, 2016

This addresses the reported issue with long messages becoming unreadable. It does two things:

  1. Add a title to each of the messages, so if it's truncated you can hover over it to see more.
  2. Add resize:horizontal to the sidebar, so you can drag-to-resize it like a textarea.

debugger-demo

@process-bot
Copy link

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@clarkware
Copy link

Very nice - thanks!

@evancz
Copy link
Member

evancz commented Nov 5, 2016

This won't solve things for more complex messages. For example, if you have Whatever "hats" 42 it will elide some information in the sidebar. It seems like showing the message on the right, above the model, is a better move. I think it should be mocked before doing work though.

@rtfeldman
Copy link
Member Author

rtfeldman commented Nov 5, 2016

if you have Whatever "hats" 42 it will elide some information in the sidebar

I don't understand why this would be true. Can you elaborate?

It seems like showing the message on the right, above the model, is a better move.

Showing a single message? If you have 40 messages down the side and they all start SetFooBarBaz ... then you'd have to scroll through each of them just to read them one at a time. That would be a painful debugging experience, especially if what you're looking for is a pattern.

Being able to scan through your messages seems important.

in
VDom.div
[ VDom.class className
, VDom.on "click" (Decode.succeed index)
]
[ VDom.span [VDom.class "messages-entry-content"] [ VDom.text (Native.Debug.messageToString msg) ]
[ VDom.span [VDom.class "messages-entry-content", VDom.title messageName ] [ VDom.text messageName ]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it seems this would not compile, because VDom.title is missing from helpers.

@gaborv
Copy link

gaborv commented Dec 15, 2016

I am trying to understand why this PR did not get accepted.
On the one hand it would be enormous help in debugging production complexity apps.
I understand it might not be the best and final solution, but is it in any ways inferior to what we have today? Does it make future changes more difficult? What is the motivation for not having this change?

@mbylstra
Copy link

Could I suggest breaking this up into two separate PRs? One for title and one for the resize:horizontal? The reason is that the title commit seems to me to be a total no brainer, and would have a higher chance of being released. It solves the immediate problem of there being absolutely no way of reading long or nested messages, which makes the debugger unusable in larger projects (where the debugger is more likely to be needed).

As an aside, is there any good reason to not have a resizable column and show the message on the right?

@mrmurphy
Copy link

mrmurphy commented Jan 4, 2017

Like @mbylstra said, I'd really love for this to be split up, and the title PR to be merged, at least.

All of my practical work happens a few layers down in the messages, so the built-in debugger is useless for my most common debugging task, which is seeing which actions were fired.

Right now I'm using https://github.com/jinjor/elm-time-travel since I can see the complete action names, but I guess my app is complicated enough that enabling that debugger significantly degrades the performance of the app.

I think that just the title addition being merged would at least let me get the info I need out of the built-in debugger.

@mrmurphy mrmurphy mentioned this pull request Jan 4, 2017
@mrmurphy
Copy link

mrmurphy commented Jan 4, 2017

I did a thing instead of making @rtfeldman do it: https://github.com/elm-lang/virtual-dom/pull/76

@rtfeldman
Copy link
Member Author

Thanks @splodingsocks! Closing in favor of #76

@rtfeldman rtfeldman closed this Jan 4, 2017
@rtfeldman rtfeldman deleted the resize-debugger branch January 4, 2017 22:32
@boxed
Copy link

boxed commented Sep 19, 2017

Nice that the hover gives you the info, but what about the pretty great resizing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants