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

adding sticky panels functionality via javascript #272

Merged
merged 3 commits into from
Dec 8, 2016

Conversation

jvanasco
Copy link
Contributor

This creates an option in the global settings panel to enable a 'sticky' behavior debug panel. If enabled, the selected panel persists across pageviews. everything is achieved via client-side javascript.

This addresses #165

A related bug/issue is #271 - the html structure and js triggering of the panels is nonstandard in toolbar.js, so this could not properly take advantage of the bootstrap API for showing them -- and is coded towards the debugtoolbar method.

Copy link
Member

@mmerickel mmerickel left a comment

Choose a reason for hiding this comment

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

I'm having difficulty understanding why this PR is the way it is.

Is there a reason that there's any options here at all? Could this instead always set a cookie with the last active panel, and try to select that panel again each time a new request is selected? There's some cases where a panel is disabled on a particular request at which point you'll need to fallback to the first panel, but that's expected even with the current approach in this PR.

@jvanasco
Copy link
Contributor Author

Is there a reason that there's any options here at all?

I prefer the sticky functionality. I think most people would.

I didn't want to introduce this "feature" at the risk of messing with anyone's current debugging process. Changing interfaces and behaviors can be jarring.

It could easily work as you suggested though.

@mmerickel
Copy link
Member

It's a debug toolbar... I have no concerns about changing the layout, especially in such a simple way. I'd prefer a simpler feature myself.

@jvanasco
Copy link
Contributor Author

ok. i'll update tonight or tomorrow.

extended the 'panel not found' behavior for failover scenarios
added a custom logging factory for development
@jvanasco
Copy link
Contributor Author

jvanasco commented Dec 5, 2016

Updated -

I set this as the default behavior and removed the toggle.
I also extended the failover behavior (documented in the Changes).
This required a bit of debug logging to get right, and I really didn't want to remove all the debug lines for future maintenance... so I ported a smaller version of a "custom logger factory" I use to the toolbar.

@mmerickel
Copy link
Member

Just tried this out and it looks good to me. One nit - I noticed that if a panel is disabled, then the first panel will be selected instead, which is great. However the previous panel is still sticky. Does my description make sense? Is this intentional? What do you think is best here? I will say I found it surprising but I was specifically looking for it.

@jvanasco
Copy link
Contributor Author

jvanasco commented Dec 8, 2016

Yep -- that's intentional (and documented in the changelog and source).

After playing around for a while, I realized that I had two general patterns when using the debugtoolbar:

  1. investigating a particular url, during which I click many tabs
  2. comparing a certain feature or bug on a collection of urls, during which I click many left-menu "page" selectors.

On that second use-case, there were many times where I'd jump from a page with an enabled tab (SqlAlchemy, logging, renderers, etc) to a page without that tab, then immediately go to a page with that tab again (and focus on it). Resetting the sticky-panel killed the utility.

Not resetting the panel is also annoying a times, but it seemed less-annoying, and in-line with the existing behavior.

I thought about overloading the jquery tabs to allow a sticky element to be set on disabled tabs, but that just seemed unneccesary.

@mmerickel
Copy link
Member

Okay, good enough for me. Thank you.

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.

2 participants