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

[WebProfilerBundle] Add channel log filter #28934

Merged
merged 1 commit into from Oct 24, 2018

Conversation

Projects
None yet
5 participants
@ro0NL
Contributor

ro0NL commented Oct 20, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...

Continuation of #28906

The 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)

image

Level filter (works the same as shown in #28906 )

image

Choice filter

image

image

We forgot to update TwigBundle previously, that's still needed after review here.

@ro0NL ro0NL force-pushed the ro0NL:log-filters branch 6 times, most recently from badf17e to 65f4668 Oct 20, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 20, 2018

@ro0NL ro0NL force-pushed the ro0NL:log-filters branch 3 times, most recently from 0e4ef17 to 1ad8533 Oct 21, 2018

@ro0NL

all good :D and a tiny step closer to sanity.

@@ -7,7 +9,7 @@
--font-sans-serif: 'Helvetica, Arial, sans-serif';
--page-background: #f9f9f9;
--color-text: #222;
--color-muted: #777;
--color-muted: #999;

This comment has been minimized.

@ro0NL

ro0NL Oct 21, 2018

Contributor

this var was overwritten below

@@ -28,7 +30,6 @@
--table-header: #e0e0e0;
--shadow: 0px 0px 1px rgba(128, 128, 128, .2);
--border: 1px solid #e0e0e0;
--color-muted: #999;

This comment has been minimized.

@ro0NL

ro0NL Oct 21, 2018

Contributor

that's here :)

@@ -892,9 +893,6 @@ tr.status-warning td {
background-color: var(--base-5);
color: var(--base-2);
}
.tab-content {

This comment has been minimized.

@ro0NL

ro0NL Oct 21, 2018

Contributor

not needed, now specified by [data-filters] (the container that requires it)

@@ -958,45 +991,6 @@ table.logs .metadata {
display: block;
font-size: 12px;
}
table.logs tr td:last-child { width: 100%; }

This comment has been minimized.

@ro0NL

ro0NL Oct 21, 2018

Contributor

already handled by .full-width on the message TH

@ro0NL ro0NL force-pushed the ro0NL:log-filters branch from 1ad8533 to 3c235b5 Oct 21, 2018

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Oct 23, 2018

Great work Roland! I've tested this in the Symfony Demo app. I only have minor remarks.

First: "Ghost filters": when loading the page, filters are displayed by default and they disappear when they shouldn't be displayed. This GIF shows a force reload in the page:

ghost-filters

Maybe it's better to "hide by default" and display when appropriate?

Second: "Moving filters":

Right now, the multiple choice filters "move" under some circumstances. See hwen I deselect all of channels and icons disappear:

channel-filter

This kind of movements are distracting and user may click wrongly because of these element displacements. What if we replace the showing/hiding icon by a form checkbox? It doesn't have to be a real form, just a clickable checkbox. Stripe does something similar:

stripe-filter

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 23, 2018

I think i've addressed both issues.

image

@fabpot

fabpot approved these changes Oct 24, 2018

@fabpot fabpot force-pushed the ro0NL:log-filters branch from 38aba04 to e1bd82e Oct 24, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Oct 24, 2018

Thank you @ro0NL.

@fabpot fabpot merged commit e1bd82e into symfony:master Oct 24, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Oct 24, 2018

feature #28934 [WebProfilerBundle] Add channel log filter (ro0NL)
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     <!-- see https://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 #28906

The 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)

![image](https://user-images.githubusercontent.com/1047696/47257162-b01bfe00-d48a-11e8-8364-d1eca69c9182.png)

Level filter (works the same as shown in #28906 )

![image](https://user-images.githubusercontent.com/1047696/47257699-78648480-d491-11e8-8c55-1dccda980de4.png)

Choice filter

![image](https://user-images.githubusercontent.com/1047696/47257205-3c2e2580-d48b-11e8-821b-e95bfed36331.png)

![image](https://user-images.githubusercontent.com/1047696/47257209-4bad6e80-d48b-11e8-8fcc-e868aa556ff8.png)

We forgot to update TwigBundle previously, that's still needed after review here.

Commits
-------

e1bd82e [WebProfilerBundle] Add channel log filter

@ro0NL ro0NL deleted the ro0NL:log-filters branch Oct 24, 2018

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

@fabpot fabpot referenced this pull request Nov 3, 2018

Closed

Release v4.2.0-BETA1 #29072

@fabpot fabpot referenced this pull request Nov 3, 2018

Merged

Release v4.2.0-BETA1 #29073

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment