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

Added Sort and Search functionality to SQLPanel #1750

Closed
wants to merge 6 commits into from

Conversation

Lakshmikanth2001
Copy link

Description

Added Sort and Search functionality to SQLPanel

Fixes # (issue)
#490

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

@Lakshmikanth2001
Copy link
Author

Issue known but don't know how to solve

  • Ignoring Eslint rule for lib javascript files such as jquery.js and dataTable
  • Adding Margin for sort and searchElements rendered by dataTable
    225699491-77ffb98c-edb2-4765-9315-8d7f8eb4cd18

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

Some people worked quite hard to remove the jQuery dependency so I'm not sure about reintroducing it for this. Also, I certainly wouldn't want to add a CDN dependency (the datatable CSS is loaded from a CDN)

Generally speaking, you'd add something like exclude: "debug_toolbar/static/debug_toolbar/js/lib/" to the top of .pre-commit-config.yaml, that should take care of making prettier and eslint skip the libraries.

I don't want to reject the contribution outright because I think it may be useful for some people, but the issue was opened a long time ago and the requested grouping of SQL queries now exists so a part of the initial issue is addressed now.

I personally probably wouldn't use something like this, because 1. I find the chronological view of SQL queries most useful to understand what's going on, 2. the browser search functionality works well to find an exact query I'm looking for and 3. the timeline already offers a quick way to find the slowest queries (and if you have so many queries that the panel loads very slowly resp. the list of queries is very long, the 1+N problem should be the first thing you look for anyway, and in this case the data table functionality doesn't seem to be that useful either to me.

I haven't started reviewing the code in detail. If we were to merge this I'd certainly want the commented out console.* statements removed. But that's nitpicking compared to the general question if we want to do this at all.

@Lakshmikanth2001
Copy link
Author

as you instructed I have omitted jQuery and dataTables dependencies and written vanilla JavaScript code for table sort which doesn't add any addition weight to your project I guess

@Lakshmikanth2001
Copy link
Author

Any updates on whether you want this feature 😐

@tim-schilling
Copy link
Contributor

@Lakshmikanth2001 I'm a -1 on this. I'm in agreement with @matthiask in regards to questioning the benefit of this. I would argue against using pagination. The original issue lists grouping as the ideal need which has been solved. And I'm not convinced sorting is needed. The horizontal bar is a good indication of what query needs attention.

The search feature I'm intrigued by. I do think it'd be neat to have a panel that could search the SQL query and code frames. For example, if I wanted to see what queries where generated by MySpecialView. Though again, I'm not sure we'd want to merge that into the toolbar immediately. It may be better off as a third-party panel that we include on the documentation.

@Lakshmikanth2001
Copy link
Author

😥 ok ; can I sort these SQL queries in views.py and then render them because I really want to see most time consuming SQL first

@tim-schilling
Copy link
Contributor

@Lakshmikanth2001 you can fork the project or create a drop-in replacement for the panel that provides that functionality.

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

3 participants