-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
UI refinements #1601
UI refinements #1601
Conversation
div.phpdebugbar-widgets-messages div.phpdebugbar-widgets-toolbar { | ||
width: calc(100% - 20px); |
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.
Normally these -20 px
were added when there is a lot of data the scrollbar appears, then a presentation problem occurred in the toolbar, maybe is a regression
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.
Perhaps many of these changes should be applied first to maximebf/php-debugbar
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.
Do you know perhaps of a specific tab that caused scrolling issues? And we're talking about an unwanted horizontal scrollbar right? I did get a horizontal scrollbar with the change you marked but I applied more changes to get rid of it.
I can certainly extract changes to the base package. I was just so far in with the changes that I thought it might make more sense to first gather feedback and then extract (if people would agree with it).
With specific changes in different PRs it would be easier to review |
This prevented the first item to be able to expand properly
Yeah I mean those indents. I'll look at the icons myself then, maybe I can just fix it upstream. Also seeing a lot of '!important` overrides, maybe it would be easier to change that in https://github.com/maximebf/php-debugbar/blob/master/src/DebugBar/Resources/debugbar.css ? |
Yeah will give it a try moving some of the changes to that package. Will take a look at it later tonight. To be continued :) |
Thanks! |
Thanks! Looks better. Should I merge #1603 first or is that conflicting? I also see that the active/highlight color is too short, but I think that was already the case 🤔 |
If you merge #1603 il take a look at the active/highlight height in this PR |
Okay merged it :) |
…nto ui-refinements
@barryvdh Yeah, looking good for me in chrome and safari. What browser are you using? Do you still have the issue? I pushed some updates to better align the dropdown in different browsers |
The dropdown is because of Filament, but I can fix that by forcing the line-height to 1em on the select. The tabs are still a bit weird, but I can add height:100% to them and it's fixed. Rest looks good, is this ready to go? |
Alright. Yeah about the dropdown, I had something similar with a specific project that had styling for select elements. The base package could benefit from having dedicated select styling, so other projects won't affect it. Will look into it. I think its ready to go yeah :) |
Thanks, fixed the height in php-debugbar/php-debugbar@424be4f |
Looks good. It's tagged now on both versions just let's see what pops up :) |
It was added to remove a 1px gap between some of the tab items and the bottom border of the header but your change also seems to do the trick, looking good at my end :) If anything arises feel free to tag me, happy to take a look. |
@@ -579,6 +597,10 @@ ul.phpdebugbar-widgets-list li.phpdebugbar-widgets-list-item .phpdebugbar-widget | |||
max-width: 100%; | |||
} | |||
|
|||
ul.phpdebugbar-widgets-list li.phpdebugbar-widgets-list-item { | |||
line-height: 20px; |
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.
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.
Also with the padding, it grows to 50px each item list on some tabs, Is it the expected behavior?
laravel-debugbar/src/Resources/laravel-debugbar.css
Lines 547 to 548 in 2f046cb
ul.phpdebugbar-widgets-list li.phpdebugbar-widgets-list-item { | |
padding: 7px 10px; |
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.
Definitely not expected behaviour, will see if I can replicate it somehow as it looks fine for me. Only thing that stands out to me is that your mono font being used is different compared to mine. Could also be a browser thing as well, could you share what you're using?
Edit: I seem to have it as well but less noticeable. Taking a look for a fix atm.
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.
Try #1604, item list down to 35px for me, but I don't know how much the expected height is
On request an other tabs i have 29px height items,
There is no longer the consistency that there was before
I'm using brave
@@ -307,13 +314,17 @@ dl.phpdebugbar-widgets-kvlist dd.phpdebugbar-widgets-value.phpdebugbar-widgets-p | |||
} | |||
|
|||
dl.phpdebugbar-widgets-kvlist dt { | |||
width: 25%; | |||
width: calc(25%-10px); |
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.
Seems like this does'nt work,
It seems strange to me, take a look https://jsfiddle.net/o5uexhvd/
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 helped to better align these items:
Fixed, only applies to Laravel debugbar. It was caused by a width/margin/padding combination of value and key element.
But you're right that's not doing anything indeed, will put it on the list to undo. The alignment of the items must've been fixed by another change.
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.
It seems that this should not be removed
div.phpdebugbar-widgets-messages div.phpdebugbar-widgets-toolbar { | ||
width: calc(100% - 20px); |
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.
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 same goes for the close button then? I expect you have this issue as well with the base package? http://phpdebugbar.com. On Mac the scrollbars hide or are half translucent when scrolling so I didn't encountered this. Is this IE or something?
Edit: Now I look at it again there is a scroll bar within your debugbar, got it.
So I've merged both your PR's. Anything missing? |
Not from my end, all good. Are the issue fixed for you @parallels999 and @erikn69 ? |
Do you have it in other apps as well. All my local apps seem to render the texts correctly |
On some places, me too, there is a new text shadow or is it my imagination? |
@@ -31,8 +31,11 @@ div.phpdebugbar code.phpdebugbar-widgets-sql span.hljs-keyword, | |||
div.phpdebugbar-openhandler .phpdebugbar-openhandler-header, | |||
div.phpdebugbar-openhandler .phpdebugbar-openhandler-header a { | |||
color: var(--color-gray-200); | |||
text-shadow: 1px 1px #000; |
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.
Maybe 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.
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 added that to make the text in the timeline waterfall better readable
Since there are many classes involved(also main div.phpdebugbar
), I saw it everywhere.
Ah the color was because of quirks mode; https://stackoverflow.com/questions/24258380/font-color-in-a-table-doesnt-inherit-from-parent-div |
@barryvdh is this expected? Seems hard to read |
Hmm no, is that because of the color inherit? |
Still not 100% aligned UPDATE: fix on 1c80f3e |
This PR contains a lot of little refinements for the ui as discussed in #1593. As I was going through the queries tab and applying changes for the spacings I noticed I couldn't let the other tabs untouched as the changes also affected those.
So I went through all the tabs and made sure everything is well aligned there as well. This results in the PR being a bit hard to review by looking at the CSS changes. Probably better to checkout the branch and compare side by side. I know you mentioned to also apply changes in the base package if applicable but I haven't checked that yet. If you like the changes in this PR I could take another look to see if anything from this PR could be extracted to the base package.
Below are screenshots of dark mode but I made sure light mode also renders correctly.
The percentages bars were being displayed with table borders but I thought it would make sense to diplay them in light/dark background as the lines in Requests tabs are displayed as well. Making it a bit more consistent.
Updated spacing and line height of the lines in the error for bit better readability
I changed the color of the 1px line of the timeline measurement to blue to match the color that is being used on the timeline tab, felt more consistent like this. Also line height has been increased slightly for the table that contains the backtrace. Probably best to compare side by side
On the requests tab the background of the key items was not extending to the bottom of the of value items. This is now fixed and look a bit cleaner I think.
Adjust colours slightly and the table is moved up slightly to reduce the gap between the filters and the table
Slight spacing improvements and removed paddings. I could make it so that only the table will scroll and not the whole modal/popup content but will need to make a change in the base package I think.
If there is anything you don't like let me know and I'll take a look.