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

Add Command to Hide Interface #1916

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nis-ship-it
Copy link

@nis-ship-it nis-ship-it commented Oct 20, 2024

Description

At the moment this code, toggles navigator, inspector, utility, pathbar and toolbar area and handles all cases for each of them when using hide interface.

Related Issues

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above

[x] I documented my code

Screenshots

Screen.Recording.2024-10-24.at.6.27.45.PM.mov

At the moment this code, toggles navigator, inspector, utility, pathbar
and toolbar area and handles all cases for each of them when using hide
interface.
@nis-ship-it nis-ship-it marked this pull request as ready for review October 21, 2024 00:48
@tom-ludwig
Copy link
Member

It looks like there’s an issue when using multiple windows:

Screen.Recording.2024-10-22.at.7.13.36.PM.mov

PS: Could you name the variables interfaceToggleLabel instead of labelForInterfaceToggle?

@nis-ship-it
Copy link
Author

@tom-ludwig tbh sorry about that I never tested it out with multiple windows

@austincondiff
Copy link
Collaborator

austincondiff commented Oct 23, 2024

@tom-ludwig I'd imagine the expected behavior here would be to only hide the interface for the current window. What do y'all think?

@austincondiff austincondiff changed the title Hide Interface - Added support Add Command to Hide Interface Oct 23, 2024
@nis-ship-it
Copy link
Author

I was only thinking for current project / window only and not multiple.

@austincondiff
Copy link
Collaborator

austincondiff commented Oct 24, 2024

@nis-ship-it could you post a screen recording demonstrating your work to the PR description when you get a chance?

@tom-ludwig
Copy link
Member

@nis-ship-it No worries! I’d expect it to only toggle the current window.

@austincondiff
Copy link
Collaborator

austincondiff commented Oct 27, 2024

Would it be possible to not animate the sidebars open/closed when hiding interface (it should animate if toggling the sidebar aside from this feature).

I also noticed that you started with the inspector closed, then you hide interface, show interface, and the inspector sidebar is open. It should be closed because you started with it closed. If it was open, then it should open.

@nis-ship-it
Copy link
Author

Sorry I was away few days, it was just that recording of the video was going to too long. For the animation, I'm not sure here what you're referring to here.

@austincondiff
Copy link
Collaborator

austincondiff commented Oct 28, 2024

@nis-ship-it I can clarify...

When the interface is hidden, it should remember the prior state so that when it’s shown again, the UI is restored to its previous state. I suggest using two variables—for instance, the inspector would be collapsed if inspectorCollapsed || interfaceHidden—so that inspectorCollapsed can be false while interfaceHidden is true, effectively hiding the inspector sidebar.

About the animation, normally when you click the sidebar toggle button in the toolbar, the sidebar animates open/closed. When hiding interface though, there should not be any animation. It should just snap open/closed.

Screen.Recording.2024-10-28.at.3.52.57.PM.mov

@austincondiff austincondiff marked this pull request as draft November 9, 2024 15:48
@austincondiff
Copy link
Collaborator

Where are we with this PR? Converting to a draft for now.

@nis-ship-it
Copy link
Author

Sorry at moment I'm away so haven't had much progress on it

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.

3 participants