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

Prevent tab borders from colliding with containing widget if show_border = false (Fixes #228) #232

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

soc
Copy link
Contributor

@soc soc commented Sep 12, 2018

The core idea of show_border = false is to get rid of the border around
the widget and set the margins to zero to visually embed the widget
directly inside the containing widget, without introducing additional
borders.

To facilitate such design choices by applications, Greybird's tabs need
to be slightly moved to the right to prevent them from colliding with
the containing widget's borders.

A screenshot of how this looks in Greybird and other themes is supplied
in #228.

// if the notebook property show_border is set to false, the frame directly inside the notebook
// doesn't exist, so we define the tab margins, the background and backdrop colors here:

> header {

Choose a reason for hiding this comment

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

Merge rule > header with rule on line 2049

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather keep this here, because of the comment above explaining why the last two rules exist.

@@ -2043,6 +2043,8 @@ notebook {
border: 1px solid shade($bg_color, 0.9);
}
}

&.frame > header { margin-left: 0; }

Choose a reason for hiding this comment

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

Merge rule &.frame > header with rule on line 2039

@@ -2043,6 +2043,8 @@ notebook {
border: 1px solid shade($bg_color, 0.9);
}
}

Choose a reason for hiding this comment

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

Line contains trailing whitespace

…der = false (Fixes shimmerproject#228)

The core idea of show_border = false is to get rid of the border around
the widget and set the margins to zero to visually embed the widget
directly inside the containing widget, without introducing additional
borders.

To facilitate such design choices by applications, Greybird's tabs need
to be slightly moved to the right to prevent them from colliding with
the containing widget's borders.

A screenshot of how this looks in Greybird and other themes is supplied
in shimmerproject#228.
> stack:not(:only-child) { // the :not(:only-child) is for "hidden" notebooks
background-color: shade($bg_color, 1.05);
border-width: 1px;
border-width: 1px 0 0;

Choose a reason for hiding this comment

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

Properties should be ordered background-color, border-color, border-style, border-width

@ochosi
Copy link
Member

ochosi commented Sep 13, 2018

Merged commit 1 of this PR to master.

@soc
Copy link
Contributor Author

soc commented May 25, 2019

pavucontrol released a new version back in March, which fixed their spacing issues:

pavu

I think the second commit would be good to go now, what do you think @ochosi?

@soc
Copy link
Contributor Author

soc commented Jul 7, 2019

This would also make the workaround in #184 unnecessary.

@ochosi
Copy link
Member

ochosi commented Sep 30, 2019

@soc Sorry for the late reply, had very little time for Greybird recently. In Xubuntu Disco the version of pavucontrol still seems to be too old, which version introduced the margin fix?

@soc
Copy link
Contributor Author

soc commented Oct 1, 2019

Hi Simon,

I think Version 4.0 should ship with the fixes:

  • Improve the use of space (remove useless margins and paddings).

@ochosi
Copy link
Member

ochosi commented Oct 1, 2019

I just checked and even the next Ubuntu release will only ship v3.0 (https://packages.ubuntu.com/search?keywords=pavucontrol&searchon=names&suite=eoan&section=all), so I'm tempted to keep this on hold for now.

@soc
Copy link
Contributor Author

soc commented Jan 1, 2020

Heads up: v4.0 is now in Debian testing, Debian unstable and Ubuntu focal.

@soc
Copy link
Contributor Author

soc commented Apr 19, 2020

@ochosi Can we get this merged?

@soc
Copy link
Contributor Author

soc commented Mar 17, 2021

Ping?

@soc
Copy link
Contributor Author

soc commented Feb 11, 2022

Well, looks like we waited for so long, that not only the application, but the whole PulseAudio thing is approaching obsolescence now.

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.

3 participants