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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Enhancement: Integrated title bar #9421

Closed

Conversation

aitor-gamarra
Copy link
Contributor

Background Info

PR Info

This PR introduces an integrated title bar for the Win32 platform, instead of the (arguably ugly and unfitting) native one.

Approaches

There are several approaches for this like the mac-like traffic-lights-on-the-header-bar approach. However, I didn't find a way I liked to do that on windows, the buttons being on the top right and such. Feel free to suggest a desing tho!

Development

I am new to Clojure, so I hope I didn't do any stupid things 馃槄

  • A new frontend component is created: win32-title-bar
  • That component communicates with the electron process in two ways:
    • Renderer $\rightarrow$ Electron: Via the ipc/ipc function, three events are sent when a button is clicked:

      1. window-minimize
      2. window-maximize-restore
      3. window-close

      These events are later handled at electron/handler.cljs.

    • Electron $\rightarrow$ Renderer: Via the web-contents.send function (window.cljs), full-screen and (un)maximize events are sent to the renderer, which are received at ui.cljs and update states accordingly.. These are needed so the renderer process knows if the app is full-screen and (un)maximized. Then, a suscription to an state is created via state/sub so components can react to changes in states.

Images

Dark Mode

w/ Sidebar

image

w/o Sidebar

image

Light Mode

Light mode images

w/ Sidebar

image

w/o Sidebar

image

Next Steps?

  • As I've said, I'm new to this, so I didn't create/modify any tests 馃槗
  • Any desing proposals you might have are welcome
  • Should this be optional? Like a toggle on the settings to switch between native / integrated title bar and/or change it's color

@CLAassistant
Copy link

CLAassistant commented May 16, 2023

CLA assistant check
All committers have signed the CLA.

@andelf andelf added :type/enhancement Enhancement to product. Does not affect the overall basic use. breaking-change labels May 16, 2023
@sprocketc sprocketc self-requested a review May 16, 2023 18:24
@cnrpman
Copy link
Collaborator

cnrpman commented May 17, 2023

@aitor-gamarra Thanks for your great work!
I also don't have any good idea of adding test on this part. So it doesn't matter.
But the authorization does 馃

Hi @MrWillCom , thanks for your contribution! Can you sign the Logseq CLA so we can introduce your code to the Logseq codebase?
Or we have to overhaul this PR instead of re-using the contribution :(

@MrWillCom
Copy link
Contributor

Hi @MrWillCom , thanks for your contribution! Can you sign the Logseq CLA so we can introduce your code to the Logseq codebase? Or we have to overhaul this PR instead of re-using the contribution :(

Okay, I've signed it. Thanks for making this cool feature come true!

@andelf
Copy link
Collaborator

andelf commented May 17, 2023

I checked this on my Windows 11. Nice work!

  • DOM level is wrong. So when the right-side-area is opened, minimize/maximize/close button is not functioning
  • Still need access to application menubar items like "Toggle Developer Tools", this can be added to the right dropdown menu.

@sprocketc
Copy link
Collaborator

sprocketc commented May 17, 2023

@aitor-gamarra Thanks a lot for this useful contribution! I would like to work on top of this, if that's OK with you.

@logseq/core Please don't merge this yet. I am working on the following

  • Merge the introduced titlebar with our existing header to save some space
  • Make it optional and available on linux
  • Fix some state/accessibility issues
  • Try to add some tests

@aitor-gamarra
Copy link
Contributor Author

@aitor-gamarra Thanks a lot for this useful contribution! I would like to work on top of this, if that's OK with you.

@logseq/core Please don't merge this yet. I am working on the following

  • Merge the introduced titlebar with our existing header to save some space
  • Make it optional and available on linux
  • Fix some state/accessibility issues
  • Try to add some tests

Fine by me and thanks to you @sprocketc! Let me know if I can help with anything :)

@sprocketc sprocketc added the :type/hold Hold this PR. won't merge for now label May 17, 2023
@sprocketc sprocketc changed the title Enhancement: Win32 integrated title bar [WIP] Enhancement: Integrated title bar May 17, 2023
@senntore
Copy link

@sprocketc would it possible for the integrated title bar to follow them native buttons like how firefox does in linux?
Asking because I prefer having only close button there.

@sprocketc
Copy link
Collaborator

@senntore Those options depend on your DE, and might also allow customizing the location of those buttons (left/right). Unfortunately, that goes beyond the scope of this PR. It would be nice if something like the titleBarOverlay was supported across all operating systems, but I don't see that happening soon.

@sprocketc
Copy link
Collaborator

Closing this in favor of #9442
@aitor-gamarra @MrWillCom Thanks again for your contribution! The follow up PR is almost ready, in case you want to test it or provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change :type/enhancement Enhancement to product. Does not affect the overall basic use. :type/hold Hold this PR. won't merge for now
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants