Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Redesigned the log message filter#28906
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| .log-levels { position: absolute; right: 5px; top: 33px; } | ||
| .log-levels { border: var(--border); box-shadow: var(--shadow); margin: 0; padding: 0; display: flex; flex-direction: column; align-items: flex-end; } | ||
| .log-levels:before { | ||
| content: "Filter"; |
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.
perhapsLevel is better (given we already know it's filter by icon), that would enable a secondChannel filter as well (which was on my list). Unless you want to combine things here of course :)
perhpslog-filters for a class, and move the content inline
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.
@ro0NL I propose you something. Given that we can no longer add a filter for the log channel in 4.2 (the "feature freeze" was a long ago, we cannot keep making changes) let's keep this filter "as is".
Later, for 4.3, we can add filters in several columns (and not only for logs). I was thinking something like GitHub filters:
Or something like "advanced data grids" which allow multi-filtering per column:
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.
We can still change things in 4.2, the 2 month period is to tweak things that are already part of the release, which is the case here.
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.
Yes ... but in my opinion what Roland is proposing (table multi-filtering) would require lots of changes.@ro0NL if you think you can do it in time for 4.2, please close this PR and implement the multi-filtering inside the table columns. Thanks 🙏 !
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.
i thought it could share the same JS basically, but that's not the case (yet).
Channel filters doesnt need the level approach (selecting all higher levels). So yeah, maybe that's something for 4.3, we'll see :)
Still, usingLevels for a label makes sense to let the user know what this filter is about ;)
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.
If that's too much work, it then makes sense to improve what we have for 4.2 and do more/better in 4.3.
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.
I've discussed about this with@ro0NL on Symfony Slack. We agree to leave this "as is" for Symfony 4.2 (so this PR is OK to be merged).
In Symfony 4.3 we strive to create a multi-purpose filter logic to add filters to all/most column headers of all/most tables. Thanks!
fabpot commentedOct 19, 2018
Thank you@javiereguiluz. |
This PR was merged into the 4.2-dev branch.Discussion----------Redesigned the log message filter| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | - <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | - <!-- required for new features -->In 4.2 we've added a log filter that looks like this:I propose a complete redesign to make it more subtle, but still useful. Filter is now displayed like this:When you click on each level, only the messages of that and higher levels are displayed ... and the other levels look disabled:| Initial | Selected| --- | ---|  | The icons display an "up arrow" and "down arrow" depending on the level to try to explain that you are rising or reducing the current level:Commits-------2271a3b Redesigned the log message filter
This PR was squashed before being merged into the 4.2-dev branch (closes#28934).Discussion----------[WebProfilerBundle] Add channel log filter| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Continuation of#28906The JS is revised to be more generic;- support 2 filter types: `level` and `choice` (respectively `Log level` and `Log channel` here)- remove default filter value support (not used yet, but opportunity kept open) - it requires a bit more work to genericify it.- filters refines the resultset (e.g. show all logs in the app channel with priority higher than alert)Level filter (works the same as shown in#28906 )Choice filterWe forgot to update TwigBundle previously, that's still needed after review here.Commits-------e1bd82e [WebProfilerBundle] Add channel log filter
In 4.2 we've added a log filter that looks like this:
I propose a complete redesign to make it more subtle, but still useful. Filter is now displayed like this:
When you click on each level, only the messages of that and higher levels are displayed ... and the other levels look disabled:
The icons display an "up arrow" and "down arrow" depending on the level to try to explain that you are rising or reducing the current level: