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

[WebProfilerBundle] Add HTTP return code in the Ajax request list table #17540

Closed
wants to merge 3 commits into from

Conversation

@kucharovic
Copy link
Contributor

kucharovic commented Jan 26, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #17518
License MIT
Doc PR -
Q | A
---|---
Bug fix? | no
New feature? | yes
BC breaks? | no
Deprecations? | no
Tests pass? | yes
Fixed tickets | #17518
License | MIT
Doc PR | -
@kucharovic kucharovic force-pushed the kucharovic:fix_17518 branch from d8043d4 to 1d5afc4 Jan 26, 2016
@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jan 26, 2016

@kucharovic I just tested your pull request and it works perfect! Thanks for working on this.

ajax_status_before

However, to make errors stand out more prominently, what do you think of using the sf-toolbar-status CSS classes that we use elsewhere in the toolbar. This way we could show the HTTP status like this:

ajax_status_after

If you like the idea, the code would be something like this:

var statusCodeCell = document.createElement('td');
var statusCode = document.createElement('span');
if (request.statusCode > 299) {
    statusCode.setAttribute('class', 'sf-toolbar-status sf-toolbar-status-red');
} else {
    statusCode.setAttribute('class', 'sf-toolbar-status');
}
statusCode.textContent = request.statusCode || '-';
statusCodeCell.appendChild(statusCode);
row.appendChild(statusCodeCell);
@kucharovic

This comment has been minimized.

Copy link
Contributor Author

kucharovic commented Jan 26, 2016

@javiereguiluz it looks better. Are you sure, that redirects should be red?

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jan 26, 2016

@kucharovic I trusted this comment from @Jean85 who said that in Ajax there are no redirections. We probably need more comments about this or some verification.

@stloyd

This comment has been minimized.

Copy link
Contributor

stloyd commented Jan 26, 2016

IMO it should be:

if (request.statusCode < 300) {
    statusCode.setAttribute('class', 'sf-toolbar-status sf-toolbar-status-green');
} else if (request.statusCode < 400) {
    statusCode.setAttribute('class', 'sf-toolbar-status sf-toolbar-status-yellow');
} else {
    statusCode.setAttribute('class', 'sf-toolbar-status sf-toolbar-status-red');
}
@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jan 26, 2016

@kucharovic anyway, to be completely safe, we could change if (request.statusCode > 299) by if (request.statusCode > 399) and this will always work no matter what.

@kucharovic

This comment has been minimized.

Copy link
Contributor Author

kucharovic commented Jan 26, 2016

@javiereguiluz he's right. A XMLHttpRequest should either get the resource or be told why not.

@stloyd

This comment has been minimized.

Copy link
Contributor

stloyd commented Jan 26, 2016

Hmmm, then maybe:

if (request.statusCode < 400) {
    statusCode.setAttribute('class', 'sf-toolbar-status sf-toolbar-status-green');
} else if (request.statusCode < 500) {
    statusCode.setAttribute('class', 'sf-toolbar-status sf-toolbar-status-yellow');
} else {
    statusCode.setAttribute('class', 'sf-toolbar-status sf-toolbar-status-red');
}
@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jan 26, 2016

@stloyd I like your proposal ... except for the last comment. Showing 4xx errors in yellow is inconsistent with the current toolbar. 4xx and 5xx are treated as errors and showed in red.

@Jean85

This comment has been minimized.

Copy link
Contributor

Jean85 commented Jan 26, 2016

Yeah, I posted that comment because I stumbled on it.. I had an issue in my code, since I was trying to catch redirects with jQuery.ajax() and I was getting always 200, didn't understand why :D

I agree with @javiereguiluz , consistency with the rest of the toolbar is better.

@Jean85

This comment has been minimized.

As per symfony#17540 (comment), this should be:

if (request.statusCode >= 400) {
    statusCode.setAttribute('class', 'sf-toolbar-status sf-toolbar-status-red');
} else {
    statusCode.setAttribute('class', 'sf-toolbar-status sf-toolbar-status-green');
}

This comment has been minimized.

Copy link
Owner Author

kucharovic replied Jan 26, 2016

@Jean85 I understood it that codes 4xx and 5xx are treated as errors and showed in red. Codes 3xx in XHR are not errors, but shouldn't be used. So code is marked as warning (yellow)

This comment has been minimized.

Copy link

Jean85 replied Jan 26, 2016

@kucharovic 3xx codes in XHR are not only never used, but never shown. The W3C specs about it talks of "transparent"; so, your browser will treat them transparently, as in you will never see the 3xx request, not even in the browser's developer tools.
So, if you will check for 3xx codes, you will have unused code, hence my comment.

This comment has been minimized.

Copy link
Owner Author

kucharovic replied Jan 26, 2016

@Jean85 never shown? They aren't shown if response is correct. If you ommit Location, browser wouldn't show destination.
out

This comment has been minimized.

Copy link

Jean85 replied Jan 26, 2016

Ah ok @kucharovic, so you're covering an edge case.. Good!

This comment has been minimized.

Copy link
Owner Author

kucharovic replied Jan 26, 2016

@Jean85 I think, a good developer tool should warn about cases like this one. So, that's why yellow color.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 26, 2016

👍

@stof

This comment has been minimized.

Copy link
Member

stof commented Jan 26, 2016

should we really put the 2xx in green or keep it in gray instead ? I think putting colors only on errors make them stand out more than if everything is colored

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jan 26, 2016

For comparison:

Color on Errors

ajax_status_after

Color on Everything

ajax_status_after_colors

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 26, 2016

+1 for gray instead of green

@maidmaid

This comment has been minimized.

Copy link
Contributor

maidmaid commented Jan 26, 2016

👍

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 27, 2016

Thank you @kucharovic.

@fabpot fabpot closed this in 1525cce Jan 27, 2016
@kucharovic kucharovic deleted the kucharovic:fix_17518 branch Jan 27, 2016
@fabpot fabpot mentioned this pull request May 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.