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

[Feature] Added a Custom Titlebar #1140

Merged
merged 3 commits into from Oct 30, 2023

Conversation

ParamBirje
Copy link
Contributor

✨ Pull Request

📓 Referenced Issue

Fixes #189

ℹ️ About the PR

As suggested by @Kidcredo , The custom-electron-toolbar package was added to the project to tweak the default toolbar. This made it possible to make the File menu and the Title be in the same space which optimizes the screen space available and more accessible for devices with smaller screen.

The app's brand color is now used by the custom titlebar.

🖼️ Testing Scenarios / Screenshots

Previous titlebar

previous_titlebar

Custom titlebar

new_titlebar

@manojVivek
Copy link
Collaborator

@ParamBirje Sorry for the delayed response, can you please try some subtle color for the background instead of the brand green?
Something default matching the windows theme should be perfect.

@ParamBirje
Copy link
Contributor Author

Hey @manojVivek , I have made the required changes to the titlebar making it's background color subtle and feel default. Here's how it looks.

image

@manojVivek
Copy link
Collaborator

Looks goods, also, is it possible to make this a user preference?

So that it doesn't affect users who like it the current way.

@ParamBirje
Copy link
Contributor Author

The custom titlebar actually builds itself during the creation of BrowserWindow, I don't think we can change it in realtime. We could probably save the user preference and on the next restart, load the preferred titlebar. I'll open a new issue/feature for changing the titlebar preferences, if that sounds good?

@manojVivek
Copy link
Collaborator

Yes, applying the change on restart sounds good (we can add a not below the user preference option indicating that).

Yes, a new issue is fine, but I would like to merge both of these together, as we wouldn't want to surprise the users with something they didn't expect.

@ParamBirje
Copy link
Contributor Author

ParamBirje commented Oct 25, 2023

Could you please give me some pointers to how the storage part be implemented? We can't use window.electron.store because the window doesn't exist yet. Should it be done using the file system (fs) API in electron or something else?

Edit: I just found a package which helps with storing electron config data. I'll try that and revert.

@manojVivek
Copy link
Collaborator

@ParamBirje You don't need any extra package for this.

You can directly access the user preferences in the main process like this:

return callback(store.get('userPreferences.allowInsecureSSLConnections'));

@ParamBirje
Copy link
Contributor Author

Hey @manojVivek ,

Thank you for the assist on electron store, saved a lot of time. Coincidently the package that I had found was electron-store itself, and then noticed it was already implemented in the project. The Titlebar Switch feature which switches between the new custom titlebar and the default titlebar has been implemented.

Custom Titlebar (default preference)

custom_titlebar

Default Titlebar

default_titlebar

Let me know if there are any changes required.

@manojVivek
Copy link
Collaborator

This is looking great!

Just a minor comment in terms of UI:
Can you please move this "Default titlebar" option into the "Settings" page instead of the menu? As this is not a frequently used option and will be a distraction if it's part of the menu.
Also, can you rename the option to something like "Menus in Title bar"? So that it's more intuitive to new users.

webPreferences: {
sandbox: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we want to disable the sandbox option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The custom-electron-titlebar mentioned it in their documentation, for the library to function properly, sandbox should be false.

However, I tested the functionality by omitting the sandbox line, and found out, the custom titlebar works just fine and encountered no issues yet.

Should I keep this line anyways?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it works without this, then I would suggest we remove this, as this is option is a compromise on security.

@@ -119,7 +125,9 @@ const createWindow = async () => {
width,
height,
icon: getAssetPath('icon.png'),
titleBarStyle: 'hidden',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, we want to ensure this doesn't affect Mac and Linux versions of the app, do you have any ideas how it works there after this change?

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 don't think this would affect Mac/Linux versions, as that option disables default taskbar styles and results in a full-size content window according to the ElectronJS docs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just tried this code on Mac and it affects the title bar color.

Preference disabled(default):
Screenshot 2023-10-29 at 11 39 21 AM

Preference enabled:
Screenshot 2023-10-29 at 11 38 42 AM

Is it possible to selectively apply this by checking, only when the platform is windows?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Try moving the styles to a new css file and optionally import based on UA data?
Screenshot 2023-10-29 at 11 53 53 AM

@ParamBirje
Copy link
Contributor Author

Hey @manojVivek ,

Here's the toggle implemented in the Settings panel.

toggle_in_settings

@ParamBirje
Copy link
Contributor Author

Hey @manojVivek ,

I have made changes to the custom titlebar's implementation such that only if the platform is Windows, the feature can be used. The UI which toggles the custom titlebar in the Settings, is now only visible to Windows users. I have also moved the custom titlebar CSS to a new file as directed.

Here's how I am checking platforms in main.ts and preload.ts

preload_main

And in the React files (process is not exposed here)

tsx_files

[The sandbox: false is now removed.]

Copy link
Collaborator

@manojVivek manojVivek left a comment

Choose a reason for hiding this comment

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

This is looking good! Thanks for patiently addressing all the comments! ♥️

@manojVivek manojVivek merged commit 212ae6e into responsively-org:main Oct 30, 2023
4 checks passed
@ParamBirje
Copy link
Contributor Author

It's been great dealing with issues and solutions back and forth. Thank you for letting me have a chance at contributing to a great project, with you and fellow maintainers doing a great job at keeping it that way! 😄

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.

[Feature] Add the File menu directly to the title bar
2 participants