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: Title bar option improvements + native title bar option for mac #854

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

khcrysalis
Copy link
Contributor

@khcrysalis khcrysalis commented Sep 14, 2024

Details

  • Hidden: Hidden titlebar, default for macOS and non-existent for other platforms
  • Native: Native titlebar, new for macOS but been present on other platforms
  • Discord: Discord's custom titlebar, default for Windows

image

NOTE: has not been tested on Windows or Linux, also my first time trying out TS! Be mindful.

@Vendicated
Copy link
Member

⚠️ Check failure on line 57 in src/renderer/components/settings/Settings.tsx ⚠️

@khcrysalis
Copy link
Contributor Author

done

transparencyOption &&
transparencyOption !== "none" && {
transparent: true
}),
...(staticTitle && { title: "Vesktop" }),
...(process.platform === "darwin" && getDarwinOptions()),
...(process.platform === "darwin" && titleBar !== "shown" && getDarwinOptions()), // Show/Hide titlebar depending on settings on Mac
Copy link
Member

Choose a reason for hiding this comment

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

getDarwinOptions() is responsible for more than just the titlebar so this will break various features

@@ -57,8 +53,11 @@ const SettingsOptions: Record<string, Array<BooleanSetting | SettingsComponent>>
title: "Enable Menu Bar",
description: "Enables the application menu bar. Press ALT to toggle visibility.",
defaultValue: false,
disabled: () => Settings.store.customTitleBar ?? isWindows
},
invisible: () => isMac,
Copy link
Member

Choose a reason for hiding this comment

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

how come?

options={[
...(isMac ? [{ label: "Hidden", value: "hidden", default: isMac }] : []),
{ label: "Native", value: "shown" },
{ label: "Discord", value: "custom", default: isWindows }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ label: "Discord", value: "custom", default: isWindows }
{ label: "Custom Discord titlebar", value: "custom", default: isWindows }

placeholder="Hidden"
options={[
...(isMac ? [{ label: "Hidden", value: "hidden", default: isMac }] : []),
{ label: "Native", value: "shown" },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ label: "Native", value: "shown" },
{ label: "Native System Titlebar", value: "shown" },

<Select
placeholder="Hidden"
options={[
...(isMac ? [{ label: "Hidden", value: "hidden", default: isMac }] : []),
Copy link
Member

Choose a reason for hiding this comment

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

i think this is what this setting means?

Suggested change
...(isMac ? [{ label: "Hidden", value: "hidden", default: isMac }] : []),
...(isMac ? [{ label: "Hidden with inset traffic light buttons", value: "hidden", default: isMac }] : []),

Comment on lines +16 to +19
<Forms.FormText className={Margins.bottom8}>
Customize apps title bar. Pick Discord if you want to use Discord's custom title bar. Requires a full
restart
</Forms.FormText>
Copy link
Member

Choose a reason for hiding this comment

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

i don't think this should be necessary if you just label the options better

@@ -20,7 +20,7 @@ export interface Settings {
appBadge?: boolean;
disableMinSize?: boolean;
clickTrayToShowHide?: boolean;
customTitleBar?: boolean;
titleBar?: "hidden" | "shown" | "custom";
Copy link
Member

Choose a reason for hiding this comment

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

the values are kinda misleading

Suggested change
titleBar?: "hidden" | "shown" | "custom";
titleBar?: "inset" | "system" | "discord";

@@ -20,7 +20,7 @@ export interface Settings {
appBadge?: boolean;
disableMinSize?: boolean;
clickTrayToShowHide?: boolean;
customTitleBar?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

when changing settings names, old settings should be migrated to the new key so users won't have their choice reset. you will likely have to do this in the main process so it applies immediately

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