-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[WebProfilerBundle] Filter links in search results #16344
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
Conversation
<td> | ||
<span class="nowrap">{{ result.ip }}</span> | ||
{% if request.session is not null %} | ||
<a href="{{ path('_profiler_search_results', request.query.all|merge({'ip': result.ip, 'token': result.token})) }}" title="Search"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title
attributes of the new links could be customized. Instead of Search
we could use any of these alternatives:
Search by IP
Filter profiles by IP
View other profiles with this IP
Show only profiles for this IP
...
Same comment for the other links.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 which one do you propose? Currently we don't use "filter" anywhere, so I sticked with "search".
@javiereguiluz like you also mentioned in #14110 (comment) ? Think someone just has to make a choice about this, the current implementation seemed to fit your concern quite well back then. |
@rvanlaak the most recent comment is only about the location of the icons. |
I find the @rvanlaak better. With your proposal @javiereguiluz, you might not see the icon as it is near the text. |
Let me know if I still need to change anything. |
Hmm, I agree with @javiereguiluz on the location of the search icon. As we use a borderless table, you would expect everything that belongs to one cell to be somewhat close to eachother. With the |
I would agree with @wouterj in the case "As we use a borderless table". But the profiler doesn't, right? |
I didn't check this PR out locally, so I gave feedback based on @javiereguiluz's screenshot and it appears to be borderless in those screenshots. If it isn't, I agree that floating right is nicer. |
Oh, I did remember to have vertical lines while developing it as well but that probably still was the So two options:
|
@rvanlaak in the new profiler design, columns don't have vertical borders (only rows have horizontal borders). I'd prefer not to change this design feature (it would take us a lot of time to tweak everything and other projects are already using this design. Example: opensoftwareconsulting/deprecation-detector#58 (comment)). |
I'm fine with any of the options, but I don't think I should make the decision about it. 👍 So shall we get rid of the float? |
I now agree with @javiereguiluz reasoning, let's do that. |
fd27ed6
to
4bffacc
Compare
Fixed 👍 |
Thank you @rvanlaak. |
…anlaak) This PR was merged into the 2.8 branch. Discussion ---------- [WebProfilerBundle] Filter links in search results | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14110 | License | MIT | Doc PR | Quite often I find myself copy/pasting the results from the search results in the profiler search form. Adding links for this allows developers to narrow down to the proper profiler result fast. Commits ------- 4bffacc [WebProfilerBundle] Filter links in search results
…lts (Rvanlaak) This PR was merged into the 2.8 branch. Discussion ---------- [WebProfilerBundle] Filter links in search results | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#14110 | License | MIT | Doc PR | Quite often I find myself copy/pasting the results from the search results in the profiler search form. Adding links for this allows developers to narrow down to the proper profiler result fast. Commits ------- 4bffacc [WebProfilerBundle] Filter links in search results
Quite often I find myself copy/pasting the results from the search results in the profiler search form. Adding links for this allows developers to narrow down to the proper profiler result fast.