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

revert using in-app menu and fallback to default window chrome #237

Merged
merged 1 commit into from Apr 6, 2020

Conversation

shiftkey
Copy link
Owner

@shiftkey shiftkey commented Mar 28, 2020

Reverts #139
Resolves #149

  • test locally and confirm menu works as expected
  • add screenshot of current chrome in default Ubuntu (with light and dark theme set)
  • rebase linux to drop original commit from future releases
  • merge PR to re-add scaffolding for mixins needed scrollbar styling - restore mixing to support custom styles targeting Linux #247

Light theme

Dark theme

I'd like to get some feedback from users that this is their desired behaviour, so please 👍 or 👎 this PR to confirm this is what you prefer. I plan to leave this open until April 11 to ensure everyone has a chance to review this.

@trxcllnt
Copy link

trxcllnt commented Mar 28, 2020

+1 for giving control of the application menu back to GTK. Here's how it looks in XFCE with the xfce4-appmenu-plugin for macos-style global application menus:

image

@alitajelsir
Copy link

I'm all with the default window chrome, but is it possible for this to be user configurable?. I think some would prefer the in-app menu. I think this will satisfy all users.

@shiftkey
Copy link
Owner Author

@alitajelsir

is it possible for this to be user configurable?

It is possible, but I want to avoid that unless there's serious interest in both states.

I think some would prefer the in-app menu. I think this will satisfy all users.

I was the one to submit this PR because of how the app looked by default (this borrowed from how the app looks on Windows, which I felt made sense for the situation), but maybe I'm in the minority? That's why I'd like feedback here...

Comment on lines -44 to -65

// A mixin which takes a content block that should only
// be applied when the current (real or emulated) operating
// system is Linux.
//
// This helper mixin is useful in so far that it allows us
// to keep platform specific styles next to cross-platform
// styles instead of splitting them out and possibly forgetting
// about them.
@mixin linux {
body.platform-linux & {
@content;
}
}

// An exact copy of the linux mixin except that it allows for
// writing base-level rules
@mixin linux-context {
body.platform-linux {
@content;
}
}
Copy link
Sponsor

Choose a reason for hiding this comment

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

While how the titlebar is styled doesn't matter to me either way, you'll likely still have some linux specific styling to apply (see my PR on styling scrollbars for dark mode), so I'd recommend keeping these mixins

Copy link
Owner Author

Choose a reason for hiding this comment

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

@jfgordon2 good point, but for now I'm going to leave this revert as-is as it makes rebasing to drop the original commit easier. I'll add the theming mixins in a follow-up PR that should support your work in #240.

This was referenced Mar 31, 2020
@shiftkey
Copy link
Owner Author

shiftkey commented Apr 4, 2020

I plan to leave this open until April 11 to ensure everyone has a chance to review this.

So far several 👍 and no objections to this change, so I might wrap this up tomorrow so I can prepare an update that contains several improvements from the previous week.

@shiftkey shiftkey merged commit 8a5b751 into linux-release-2.4.0 Apr 6, 2020
@shiftkey shiftkey deleted the revert-menu-styling branch April 6, 2020 15:07
@harry-cpp
Copy link

Well I'm late, I personally preferred the CSD version, the non CSD version looks quite ugly on GNOME.

@shiftkey
Copy link
Owner Author

shiftkey commented Jun 1, 2020

@harry-cpp the discussion is continuing over in #285 if you'd like to share more about your setup

@harry-cpp
Copy link

harry-cpp commented Jun 1, 2020

You linked to an issue about a bug with menus and fullscreen? Is that the right link?

@shiftkey
Copy link
Owner Author

shiftkey commented Jun 1, 2020

@harry-cpp off by one - I meant #285 - apologies

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

5 participants