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

Fix context menu styling #890

Merged
merged 2 commits into from
Oct 10, 2024
Merged

Fix context menu styling #890

merged 2 commits into from
Oct 10, 2024

Conversation

git-f0x
Copy link
Contributor

@git-f0x git-f0x commented Sep 28, 2024

This makes the menu match other context menus in COSMIC, and ensures some spacing between the shortcut and title. Tried to make the menu change with Density (hence the libcosmic update), but it didn't seem to change anything, regardless of the way of incorporating the spacing variables.
screenshot-2024-09-28-11-54-22
(Edit: Not sure why the text color of the title is the same as the shortcut. I've tried switching the title back to how it was, but it didn't change. But this is also the case in menus in apps)

Also tweaked the resize indicators for better styling. The right indicator still gets cut off slightly at smaller window widths, but not sure why that happens.

The centering of the SSD header title has a minor issue in some apps at minimum widths (only saw it on GitHub desktop so far; other apps have higher minimum widths), but not sure how that can be avoided. Changing the header_bar in libcosmic can improve this, but it also messes up COSMIC app headers.
screenshot-2024-10-08-10-37-47

Also improves the styling of resize indicators, and removes the unnecessary `.density()` method call for the SSD header.
@Drakulix
Copy link
Member

Drakulix commented Oct 8, 2024

This makes the menu match other context menus in COSMIC, and ensures some spacing between the shortcut and title.

Thanks for working on this!

Tried to make the menu change with Density (hence the libcosmic update), but it didn't seem to change anything, regardless of the way of incorporating the spacing variables.

Possibly this isn't reload correctly (or even initially), since we can't rely on libcosmic's App implementation to do that for us. E.g.: https://github.com/pop-os/cosmic-comp/blob/master/src/shell/mod.rs#L3700

Also tweaked the resize indicators for better styling. The right indicator still gets cut off slightly at smaller window widths, but not sure why that happens.

If that is not a new problem we can ignore that for now.

(hence the libcosmic update)

Libcosmic updates have broken things in cosmic comp in the past given the unusual integration.
It seems like you already tested a lot of the UI elements, but please make sure you tested all of them:

  • Window decorations
  • Stack decorations
  • Context menu
  • Resize indicator
  • Swap indicator (Triggered in tiling with the swap shortcut)
  • Stack indicator (Triggered in tiling when drag & dropping a window on top of another one)

@git-f0x
Copy link
Contributor Author

git-f0x commented Oct 8, 2024

Everything listed seems to work as expected. :)
Another issue that was present before (and still is), is that the Resize context menu item doesn't extend to the edges and has less height, compared to other items (as if it has some kind of padding around itself). But that's just a minor visual issue.
Edit: Setting this to 0.0 makes the item look as expected (but looking at the button style in libcosmic, it should already be 0.0?). Should that be changed?

@Drakulix
Copy link
Member

Everything listed seems to work as expected. :)

Great. Good for merge then imo.

Another issue that was present before (and still is), is that the Resize context menu item doesn't extend to the edges and has less height, compared to other items (as if it has some kind of padding around itself). But that's just a minor visual issue. Edit: Setting this to 0.0 makes the item look as expected (but looking at the button style in libcosmic, it should already be 0.0?). Should that be changed?

If it works with explicit 0.0 and not otherwise, I would rather figure out, why the theme is giving us wrong values here. But that can be done in another PR.

@Drakulix Drakulix merged commit ea2215e into pop-os:master Oct 10, 2024
1 check passed
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.

2 participants