-
Notifications
You must be signed in to change notification settings - Fork 49
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
profiler redesign #146
profiler redesign #146
Conversation
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 have nothing to remark.
Good work
Could you have a look why the tests are failing? |
I broke those myself and didn't update them yet. About the features, I didn't re-implemented the display of failed stacks. I don't think the http status is enough as any plugin can transform an 200 response to an exception. I also removed the path of arrows in the plugin stack. Maybe it must be re-introduced for users not familiar with PluginClient internals. |
Should I follow php-http/client-common#45? |
Yes. That is correct. They are just value objects. It is okey to use them when testing. |
So I can consider Promise a value object? |
Great work @fbourigault thanks for your hard work. Regarding promises: while messages can be considered VOs, Promises not really. They represent behaviour, not values. |
Does someone have ideas on how to display a failed stack? |
Thank you Mark. @fbourigault: Hm, No, not really. Maybe just display the stack as normal but on the request line indicate it failed. |
I think this is now ready. I encourage everyone to test this new profiler on their own projects. Using the profiler worth more than my screenshots. |
Thank you. I will test it and see what's happen.. I maybe have time to do it before Tuesday. |
I ran some extra tests using BrowserStack. It works on every browser (IE, Edge, Chrome, Firefox, Safari) where the profiler is fine 👌 |
I've tried the PR now. I like it. I've got nothing to complain about. @fbourigault Really good job. Feel free to merge when you are ready. |
Here is my proposal for the new profiler design.
Now, when you display the profiler, you just see a list of requests sent and only have the method, the url and the response status code displayed. When you click on a line, the request (captured after plugin stack) and the response (captured before the plugin stack) are displayed. Then you have a button to toggle plugin stack which show the plugins list.
I used CSS3 flexbox for the layout with the help of word-wrap & co, we get a nice look at every screen size. No overflow for very long headers/urls.
The body toggle is now moved per request profile instead of globally.
I also cleaned the html to be more CSP friendly (no more inlined style/script attributes). I also reduced the javascript code to the strict minimum.
HTTP method colors are from SwaggerUi. I also used the best font color to have enough contrast for a11y.
Comments are welcome :)
To Do