minbrowser / min

A fast, minimal browser that protects your privacy
https://minbrowser.org/
Apache License 2.0
7.9k stars 704 forks source link

Fix/tab item alignment #2222

Closed RubenSmn closed 1 year ago

RubenSmn commented 1 year ago

Description

Improves the item alignment in a tab.

Changes made:

Before
_home_ruben_Development_forks_min_index html
After
_home_ruben_Development_forks_min_index html

I forced some active states to show the icons.

PalmerAL commented 1 year ago

Thanks for the PR! It's odd things are so misaligned in your original screenshot - those buttons are mostly centered on macOS, and I recall them looking much better than that on Windows and Linux also. Switching to flexbox is a good solution though.

Two bugs I've noticed so far:

The hover effect on the tab close button is broken:

Screenshot 2023-04-25 at 11 58 50 PM

There's something weird going on with the layout of tabs with permission icons when selected:

https://user-images.githubusercontent.com/10314059/234474109-6e8cded2-62e4-41df-ac2c-d1f409db8fb2.mov

RubenSmn commented 1 year ago

Hey thanks for the feedback! I'll get that fixed.

Might be worth mentioning I'm using Linux: ZorinOS

RubenSmn commented 1 year ago

@PalmerAL as you might be able to tell from the screenshots the permission icon changed a bit, its not fully round anymore. I just updated this, its now round again. :+1:

I've also made some change to how the hover effect is positioned. Before the hover effect was very off, same as you displayed in the video. I hope this fix will be compatible on both sides.

PalmerAL commented 1 year ago

Sorry for taking so long to reply. This looks better! Both fixes look good.

There are a couple spacing issues left that I see:

There should be a bit more space inside the tab to the left of "New Tab":

Screenshot 2023-05-08 at 5 54 30 PM

There's too much space to the right of the icon, and there should be space inside the tab after "news":

Screenshot 2023-05-08 at 5 54 07 PM

The back and forward buttons have too much space between them:

Screenshot 2023-05-08 at 5 55 33 PM
RubenSmn commented 1 year ago

I've fixed the issues @PalmerAL.

Let me know if there are anymore improvements I can make!

PalmerAL commented 1 year ago

Sorry, two more things:

  1. The spacing seems broken when the tab is wider than the title and there are icons:
Screenshot 2023-05-12 at 9 13 02 PM

Removing the justify-content: space-between on the tab item sort of fixes this, but then the close button is in the wrong place:

Screenshot 2023-05-12 at 9 18 11 PM
  1. Previously, the forward button would appear with an animation - how difficult would it be to add this back?

https://github.com/minbrowser/min/assets/10314059/f0b3208c-2199-4c2a-baca-55473614be22

RubenSmn commented 1 year ago

No need for any apology, thank you for finding these edge cases. I found 2 others myself

I've fixed the title spacing and the placement of the close button. Changed the fixed width of the navigation buttons to a more appropriate size (was occupying unnecessary space) which brings back the transition / animation.

PalmerAL commented 1 year ago

This looks really good! I only see one minor issue left: the spacing on the sides of the tab item is a bit uneven, because the gap applies to the tab-icon-area even when no icons are showing:

Screenshot 2023-05-16 at 9 42 19 PM

This means an extra character or two of the tab title is hidden when it doesn't need to be. It's not a huge issue though, if you don't see a simple way of fixing it (I don't at the moment) I'm fine leaving it as-is.

RubenSmn commented 1 year ago

Great! I was going to solve this by checking if the tab-icon-area had no children and then apply the gap width as negative margin. But I found that it always has a i element as a secure indicator, so now wer'e checking if the area only contains the i element and then applying the style.

PalmerAL commented 1 year ago

I don't think that works, because the icon area also contains the tab close button, but what do you think of the change in 1b679548bd5e3bd1b2136b843e2b98e3bd44ba57 ?

RubenSmn commented 1 year ago

Ah okay, this works better but I'm having a issue with hovering. Might be something just on linux as I'm not seeing the close icon on your tabs and I assume they probably appear when hovering a tab.

Kooha-2023-05-18-11:58:23.webm

PalmerAL commented 1 year ago

Do you have a touch screen? That'll make the close buttons appear all the time.

In that case, the rule should be something like:

body:not(.touch) .tab-item:not(:hover) .tab-icon-area:has(.tab-close-button:only-child)
RubenSmn commented 1 year ago

Yes I have a touch screen, the rule you've provided does the trick @PalmerAL!

I couldn't find anymore issues, let me know if you need any adjustments.

PalmerAL commented 1 year ago

It all looks good to me! Thanks for all the revisions on this!