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

UI refinements #1601

Merged
merged 16 commits into from
Apr 3, 2024
Merged

UI refinements #1601

merged 16 commits into from
Apr 3, 2024

Conversation

nckrtl
Copy link
Contributor

@nckrtl nckrtl commented Apr 1, 2024

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.

Screenshot 2024-04-01 at 23 07 49

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.

Screenshot 2024-04-01 at 23 08 08

Updated spacing and line height of the lines in the error for bit better readability

Screenshot 2024-04-01 at 23 08 56

Screenshot 2024-04-01 at 23 09 33

Screenshot 2024-04-01 at 23 09 47

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

Screenshot 2024-04-01 at 23 10 12

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.

Screenshot 2024-04-01 at 23 10 47

Adjust colours slightly and the table is moved up slightly to reduce the gap between the filters and the table

Screenshot 2024-04-01 at 23 11 03

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.

Screenshot 2024-04-01 at 23 11 18

If there is anything you don't like let me know and I'll take a look.

div.phpdebugbar-widgets-messages div.phpdebugbar-widgets-toolbar {
width: calc(100% - 20px);
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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

@parallels999
Copy link
Contributor

Updated spacing and line height of the lines in the error for bit better readability

image
You break it

@parallels999
Copy link
Contributor

parallels999 commented Apr 1, 2024

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.

In this way, the final results can no longer be differentiated much from the timeline waterfall.
They can be confused as concurrent(yes could be concurrent, try views timeline)
image

@parallels999
Copy link
Contributor

With specific changes in different PRs it would be easier to review

@nckrtl
Copy link
Contributor Author

nckrtl commented Apr 2, 2024

Updated spacing and line height of the lines in the error for bit better readability

image You break it

Fixed
Screenshot 2024-04-02 at 09 00 07

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.

In this way, the final results can no longer be differentiated much from the timeline waterfall. They can be confused as concurrent(yes could be concurrent, try views timeline) image

Brought back table borders:
Screenshot 2024-04-02 at 09 24 28

With specific changes in different PRs it would be easier to review

I could separate the changes in different PR's. Will need to extract some changes to the base package anyways as you mentioned that'd make more sense. But will do that once it's clear that these changes are considered an improvement. Waiting for more feedback for now.

@barryvdh
Copy link
Owner

barryvdh commented Apr 2, 2024

I really like the query change, so that it's not the entire background :)

There seems to be some weird spacing if an item has multiple lines. Also visible on your screenshot, in the request_headers row.

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.

Screenshot 2024-04-01 at 23 10 47

Same with multiple middleware:
image

I see you changed the icons, but the close and browse icons still feel 'fuzzy' vs the other icons
image

Not sure if that was intended to change or not.

@nckrtl
Copy link
Contributor Author

nckrtl commented Apr 2, 2024

Thanks for the feedback and giving it a spin. Regarding the spacing you mean these spacings/indent right?
318571919-4c7dfb98-9a62-4da1-affd-7f63aa5dd206-2
Seems it also has weird indents/spacing in the current version but my changes seem to have intensified the spacing. Will take a look at it. The middleware example is clear, will try and reproduce this.

I didn't change the icons actually. I adjusted the positioning but as the change happened on the same line as the base64 encoded value the whole line is seen by git to be updated I think. The fuzziness is being caused by the icons being base64 enconded PNG's, not SVGs. I can replace them for svg's which should solve the fuzziness.

@barryvdh
Copy link
Owner

barryvdh commented Apr 2, 2024

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 ?

@nckrtl
Copy link
Contributor Author

nckrtl commented Apr 2, 2024

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

@barryvdh
Copy link
Owner

barryvdh commented Apr 2, 2024

Thanks!
For icons, I created maximebf/php-debugbar#644 which replaces the svgs with font-awesome icons, so it's more inline with the rest (and easier to change color)

@nckrtl
Copy link
Contributor Author

nckrtl commented Apr 3, 2024

Thanks for the feedback and giving it a spin. Regarding the spacing you mean these spacings/indent right? 318571919-4c7dfb98-9a62-4da1-affd-7f63aa5dd206-2 Seems it also has weird indents/spacing in the current version but my changes seem to have intensified the spacing. Will take a look at it. The middleware example is clear, will try and reproduce this.

I didn't change the icons actually. I adjusted the positioning but as the change happened on the same line as the base64 encoded value the whole line is seen by git to be updated I think. The fuzziness is being caused by the icons being base64 enconded PNG's, not SVGs. I can replace them for svg's which should solve the fuzziness.

Fixed, only applies to Laravel debugbar. It was caused by a width/margin/padding combination of value and key element.

Screenshot 2024-04-03 at 09 50 40

@barryvdh barryvdh mentioned this pull request Apr 3, 2024
@barryvdh
Copy link
Owner

barryvdh commented Apr 3, 2024

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 🤔
image

@nckrtl
Copy link
Contributor Author

nckrtl commented Apr 3, 2024

If you merge #1603 il take a look at the active/highlight height in this PR

@barryvdh
Copy link
Owner

barryvdh commented Apr 3, 2024

Okay merged it :)

@nckrtl
Copy link
Contributor Author

nckrtl commented Apr 3, 2024

My latest commit is a bit messy but did the following things:

  • Replaced the Laravel icon for an SVG version, just to be more consistent with the other icon formats
  • Fixed the active tab height. Also no more gap between bottom header bar and other header items/tabs:
Screenshot 2024-04-03 at 11 51 18 - Also applied some more alignment of the logo to better align with tab content - Text is also visually more centered in the tabs in combination with the pills/counters

@barryvdh
Copy link
Owner

barryvdh commented Apr 3, 2024

Hmm it slight different with me. This is with dev-master of php-debugbar right?
image

On the right side it's correct
image

Only the dropdown is a bit crushed

@nckrtl
Copy link
Contributor Author

nckrtl commented Apr 3, 2024

@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

@barryvdh
Copy link
Owner

barryvdh commented Apr 3, 2024

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?

@nckrtl
Copy link
Contributor Author

nckrtl commented Apr 3, 2024

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

@barryvdh barryvdh merged commit 2f046cb into barryvdh:master Apr 3, 2024
25 checks passed
@barryvdh
Copy link
Owner

barryvdh commented Apr 3, 2024

Thanks, fixed the height in maximebf/php-debugbar@424be4f
Not sure why the 16px was needed, because it conflicted for me and doesn't seem to be required.

@barryvdh
Copy link
Owner

barryvdh commented Apr 3, 2024

Looks good. It's tagged now on both versions just let's see what pops up :)

@nckrtl
Copy link
Contributor Author

nckrtl commented Apr 3, 2024

Thanks, fixed the height in maximebf/php-debugbar@424be4f Not sure why the 16px was needed, because it conflicted for me and doesn't seem to be required.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this change seems like messages list grows too much on gate tab, and the icons are misaligned
image
Same on messages tab
image

Why don't just normal line height??

Copy link
Contributor

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?

ul.phpdebugbar-widgets-list li.phpdebugbar-widgets-list-item {
padding: 7px 10px;

Copy link
Contributor Author

@nckrtl nckrtl Apr 3, 2024

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.

Copy link
Contributor

@erikn69 erikn69 Apr 3, 2024

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

@erikn69 erikn69 mentioned this pull request Apr 3, 2024
@@ -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);
Copy link
Contributor

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/

Copy link
Contributor Author

@nckrtl nckrtl Apr 3, 2024

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.

Screenshot 2024-04-03 at 09 50 40

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.

Copy link
Contributor

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

#1605 (comment)

div.phpdebugbar-widgets-messages div.phpdebugbar-widgets-toolbar {
width: calc(100% - 20px);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a regression,
the -20px was there to prevent the toolbar from obstructing the vertical scrollbar, on all the tabs extending messages

image

image

Copy link
Contributor Author

@nckrtl nckrtl Apr 3, 2024

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.

@barryvdh
Copy link
Owner

barryvdh commented Apr 3, 2024

So I've merged both your PR's. Anything missing?

@nckrtl
Copy link
Contributor Author

nckrtl commented Apr 3, 2024

Not from my end, all good. Are the issue fixed for you @parallels999 and @erikn69 ?

@barryvdh
Copy link
Owner

barryvdh commented Apr 3, 2024

I do see that the color is off, not sure why that is:

image

And this link is not readable
image

@nckrtl
Copy link
Contributor Author

nckrtl commented Apr 3, 2024

Hmm can't seem to reproduce this, what looks like for me on the latest version and on master:

Screenshot 2024-04-03 at 19 45 35 Screenshot 2024-04-03 at 19 45 29

On page load I do see "View in Telescope" in blue for a split second and then it goes to the correct color.

@nckrtl
Copy link
Contributor Author

nckrtl commented Apr 3, 2024

Do you have it in other apps as well. All my local apps seem to render the texts correctly

@erikn69
Copy link
Contributor

erikn69 commented Apr 3, 2024

I do see that the color is off, not sure why that is:

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe here??

Copy link
Contributor Author

@nckrtl nckrtl Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I added that to make the text in the timeline waterfall better readable on blue background:
Screenshot 2024-04-03 at 20 22 30

With the text shadow:
Screenshot 2024-04-03 at 20 21 11

But quite some elements are affected by that change, perhaps too many:
Screenshot 2024-04-03 at 20 28 27

Copy link
Contributor

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.

@nckrtl nckrtl mentioned this pull request Apr 3, 2024
@barryvdh
Copy link
Owner

barryvdh commented Apr 3, 2024

@erikn69
Copy link
Contributor

erikn69 commented Apr 3, 2024

@barryvdh is this expected? Seems hard to read
image

@barryvdh
Copy link
Owner

barryvdh commented Apr 3, 2024

Hmm no, is that because of the color inherit?

@erikn69
Copy link
Contributor

erikn69 commented Apr 3, 2024

is that because of the color inherit?

No, it seems that it was already there, it happens in dark theme

image

If you add color: white; there
image

image

@erikn69 erikn69 mentioned this pull request Apr 3, 2024
@parallels999
Copy link
Contributor

parallels999 commented Apr 12, 2024

Still not 100% aligned
image
Needs padding-top: 0;

UPDATE: fix on 1c80f3e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants