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

feat: active border radius applies to stack tab buttons as well #1596

Merged
merged 5 commits into from
Mar 22, 2023

Conversation

BobbyByrne
Copy link
Contributor

Given how rounded corners are becoming more popular and we allow the Active Hint's border radius to be changed, I felt like the stack tab buttons needed to match the border radius of the active hint.

image

Originally I was hoping to only do the outside buttons but I don't believe the CSS options are supported yet (example: border-top-left-radius)

@n3m0-22 n3m0-22 self-assigned this Mar 17, 2023
@n3m0-22 n3m0-22 requested review from a team March 17, 2023 15:27
@BobbyByrne
Copy link
Contributor Author

Just as a side note, I did try out doubling the tab height and moving the tabs behind the windows giving the tabs the appearance of hugging the contours of the overlapped window. It works, but when moving windows off of a stack, the tabs become exposed and sit in-front of the windows in the stack until a window is activated on the stack. Figured this is a simpler solution that works with the existing stack/tab button arrangement logic.

n3m0-22
n3m0-22 previously approved these changes Mar 17, 2023
Copy link
Contributor

@n3m0-22 n3m0-22 left a comment

Choose a reason for hiding this comment

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

All regression testing has passed.

shell-testing.txt

Note: During testing I came across an issue with Super+G. It is not a regression caused by this PR. I will link it here so this is not mistakenly thought to be the cause of the issue.

#1597

@maria-komarova
Copy link

maria-komarova commented Mar 17, 2023

Looking at the screenshot on the PR, seems like there are some nice improvements that align with our new designs for window stacks in Cosmic: pop-os/cosmic-epoch#39. In particular, the addition of the app icon could help distinguish between the windows in a stack at a glance.
In terms of styling, we're not actually using pill-shaped buttons or any other UI elements in Pop!_OS 22.04. While it is true that a lot of people prefer rounded corners because they seem more inviting and friendlier, and this is also a direction we take with new Cosmic, I'm hesitant to introduce this to Pop!_OS 22.04. It might add more visual inconsistency across various elements of the UI and make the stack headers seem more out of place. Using pill shapes for the tabs also leaves a lot of small transparent areas between them and on the sides in the stack header. It might be better to keep stack headers as a more unified element with one solid background while using dividers between tabs and a different background for the currently focused window.
I'm not sure about the window-close button in each window tab. Are those buttons part of the PR, actually?
Things that we should check, might need screenshots of:

  • how do the labels inside the window tabs look when there are, let's say 12-15 windows open.

@jacobgkau
Copy link
Member

In particular, the addition of the app icon could help distinguish between the windows in a stack at a glance.

I'm not sure about the window-close button in each window tab. Are those buttons part of the PR, actually?

Neither of these are new with this PR. Tabs already have icons and close buttons (below is what's currently released.)

Screenshot from 2023-03-17 16-03-05

This PR is just changing the shape of the tabs.

I agree there's a lot of space between tabs with this change.

@maria-komarova
Copy link

Thanks for the clarification, ignore my comments about icon and close-button.

@jacobgkau
Copy link
Member

how do the labels inside the window tabs look when there are, let's say 12-15 windows open.

Here is 12 windows (single stack on a 1080p display):

Screenshot from 2023-03-17 16-28-36

Here is 15:

Screenshot from 2023-03-17 16-28-29

@maria-komarova
Copy link

maria-komarova commented Mar 17, 2023

Thanks for the screenshots. Thoughts about visual consistency throughout the system are the ones that make me hesitate. And it sounds like just rounding the tab top corners that touch the edges is not an option:

Originally I was hoping to only do the outside buttons but I don't believe the CSS options are supported yet (example: border-top-left-radius)

Wonder if it would take rewriting some of the code to make it look more consistent with other things in the UI.

@n3m0-22
Copy link
Contributor

n3m0-22 commented Mar 17, 2023

For the sake of clarity this only changes the radius on the stack tabs when Active Border Radius is changed. The following is an example of the changes this makes with Active Border Radius set to 30, 20 , 10, 5 (default), and with 15 tabs open and Active Border Radius set to 16.

stacks

@maria-komarova
Copy link

maria-komarova commented Mar 17, 2023

Does anyone know if there is any way to only round top right and left corners of the outer tabs respectively? That might still be a much better solution visually and in terms of UI consistency and cohesive feel.

@BobbyByrne
Copy link
Contributor Author

Does anyone know if there is any way to only round top right and left corners of the outer tabs respectively? That might still be a much better solution visually and in terms of UI consistency and cohesive feel.

I believe I can modify the code to only add a radius to the outside corners of the first and last tab buttons. I feel like it would look best if it was both the top and bottom left corners had a radius on the first tab (top and bottom right for the last) since a sharp bottom corner would look strange on top of the window with a rounded corner. But removing the rounded corners on the tab sides that have tab neighbours makes sense.

Just working on the changes now. Will probably update the PR over the weekend.

@BobbyByrne
Copy link
Contributor Author

@maria-komarova, following @n3m0-22's screenshot variations, here's the new changes. Since the purpose of this feature is to better match the Active Hint window outline when it has a radius set (likely to match the current theme and it's window corner radius settings), having a rounded top-left and rounded top-right while also having a sharp bottom-left and bottom-right would look strange IMO. You can see why on the 0px radius example below where the window has a radius but the bottom tab corners don't. So I'm meeting in the middle with this iteration. Below is the updates with 0, 5, 12, 16, and 32px radius. 32 just shows the limitations of having a tab height of only 24px and the 16 option is the default in gnome's default Adwaita theme.

Screenshot from 2023-03-18 11-21-32
Screenshot from 2023-03-18 11-22-26
Screenshot from 2023-03-18 11-22-57
Screenshot from 2023-03-18 11-23-11
Screenshot from 2023-03-18 11-24-06

Another option which requires more work and potential logic changes, would be to have the tab height be dynamic based on the height of the radius. The window position wouldn't change so the tab height would still look like 24px, but the actual height would be hidden behind the window. This would give the illusion of the tab hugging the windows rounded corners to a point along the side. Image example below.

image

Let me know your thoughts :)

@BobbyByrne
Copy link
Contributor Author

I should also mention that in the recent batch of screenshots I'm using a window that has a corner radius of 16px. It might be better to see the effect of the Pop theme's 5px corner.

@maria-komarova
Copy link

I think that looks much better. It helps to have less fragmented window stack header since the tabs in the middle retain their 0 corner radii.
You're right, it would be helpful to see the screenshot with the Pop theme window to make sure it works there as well.

Another option which requires more work and potential logic changes, would be to have the tab height be dynamic based on the height of the radius. The window position wouldn't change so the tab height would still look like 24px, but the actual height would be hidden behind the window. This would give the illusion of the tab hugging the windows rounded corners to a point along the side. Image example below.

Right, that could be another option. The tabs might need to change for this one a bit too. Most likely we'll need to indent the first and last tabs so that the corners of the window stack header are background rather than tabs. That way all the tabs will remain uniform. In the new Cosmic DE designs we used that space to place an icon representing the stack and to ensure there is space in the stack header that one can "grab" with the mouse to drag the entire stack around:
image
The dragging of the stack is a new feature still in the works though so it's probably not something we'll have in pop-shell. So if we have a similar solid background edges of the window stack headers it would be mostly for aesthetic purposes in this case. At least for now.

@BobbyByrne
Copy link
Contributor Author

@maria-komarova, I agree and you've added another reason to look forward to Cosmic. I'll attach some additional screenshots of the current iteration using the 5px radius on the windows and the tabs. I've also done some initial investigation into the "hugging" tabs and I think it's a better design, but there's a fair amount of changes required that it warrants a separate PR. If I find the time to implement that improved version of the tabs design, I'll link to this PR to continue the conversation.

@BobbyByrne
Copy link
Contributor Author

Here's an example of the pop-theme's default 5px radius on a window and 5px set on the Active Hint corner radius. I've dropped the resolution down to 720p for a closer view. There was always a small gap between the tab and window in the corner, but now it looks a bit more intentional.

Screenshot from 2023-03-21 00-07-09

@BobbyByrne
Copy link
Contributor Author

Side question: is there a proper way to test changes to the pop-shell in Wayland? I tried following the docs guidelines here, but I don't know how to trigger keyboard shortcuts in the nested wayland session. Logging out and back in was a bit annoying while developing lol.

Copy link

@maria-komarova maria-komarova left a comment

Choose a reason for hiding this comment

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

UX is good with the changes unless someone else has any concerns.

@maria-komarova
Copy link

@BobbyByrne thanks for working through the best solution to fix the rounded corner discrepancy and for the contribution :)

@n3m0-22 n3m0-22 self-requested a review March 21, 2023 17:09
Copy link
Contributor

@n3m0-22 n3m0-22 left a comment

Choose a reason for hiding this comment

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

With the most recent commit all testing is still passing. Looks good to me.

@mmstick mmstick merged commit fab66e1 into pop-os:master_jammy Mar 22, 2023
@BobbyByrne BobbyByrne deleted the feature/tab-border-radius branch March 24, 2023 18:35
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.

5 participants