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

workspace: Implement Extended Terminal Layout #21337

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

Conversation

th0jensen
Copy link

@th0jensen th0jensen commented Nov 29, 2024

Closes #10211

Screenshot 2024-11-29 at 4 13 54 PM

Release Notes:

  • Added new layout tree for the bottom dock/terminal to have a full layout that extends below the left and right docks.
  • Added configuration under bottom_dock_layout -> "contained" | "full"
  • Added button in the terminal toolbar to toggle the behaviour quickly.
  • Action (Workspace::ToggleBottomDockLayout)

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Nov 29, 2024
@th0jensen th0jensen changed the title Initial implementation of full bottom dock layout Add option to extend Bottom Dock below the Left and Right Docks Nov 29, 2024
@th0jensen th0jensen marked this pull request as draft November 29, 2024 20:44
@th0jensen th0jensen changed the title Add option to extend Bottom Dock below the Left and Right Docks workspace: Implement Extended Terminal Layout Dec 1, 2024
@th0jensen th0jensen marked this pull request as ready for review December 1, 2024 13:20
@mikayla-maki
Copy link
Contributor

What is the source for the expand asset you're using in this PR?

@mikayla-maki mikayla-maki self-assigned this Dec 6, 2024
@th0jensen
Copy link
Author

What is the source for the expand asset you're using in this PR?

I just asked Zed AI for an SVG that was free to use and would match Zed's other icons in terms of style. I think the decision for which icon to use is ultimately up to you guys, whether this icon fits in or not.

Copy link
Contributor

@mikayla-maki mikayla-maki left a comment

Choose a reason for hiding this comment

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

Wow, the AI did a great job on that icon.

That said, we prefer to use Lucide icons when we need new UI buttons. I added some notes about which ones I think we should use and some implementation details.

crates/workspace/src/dock.rs Outdated Show resolved Hide resolved
assets/icons/expand_horizontal.svg Outdated Show resolved Hide resolved
crates/terminal_view/src/terminal_panel.rs Outdated Show resolved Hide resolved
@th0jensen
Copy link
Author

Thanks for the input on the icons! I moved the property that was on the dock to the workspace, as well as changing out the icons :)

@mikayla-maki
Copy link
Contributor

mikayla-maki commented Dec 14, 2024

Awesome! Looks like there's a merge conflict, and could you include a screenshot showing the button you added?

Also, I like that we could add a left-contained setting for this as well, so that only one of the side bars is full width.

@th0jensen
Copy link
Author

After adding the left-contained variant I decided to add in both alignments so people can choose what they prefer. So I thought this was the best way to implement the button, but I'm not quite sure. I encountered a lot of "Rustisms" I didn't quite understand, but hey, it runs!

I haven't noticed any weird behaviours or bugs when testing the builds, but I feel like the implementation in crate/terminal_view/terminal_panel.rs is a bit non-optimal.

This is what the button looks like:
Screenshot 2024-12-17 at 11 08 24 PM

@th0jensen
Copy link
Author

I also feel like I could use the lucide icon columns-3 and shade which space the bottom bar will occupy. I'd display this next to the labels in the dropdown menu, but it would require adding a new method to crate/ui/context_menu.rs:

The method toggleable_entry() already has a configurable icon: None, so I imagine I could just expose that as a parameter in a different method, something like toggleable_entry_with_icon?

This may be too much effort for a feature that I imagine most people will just configure in settings and leave be...

@mikayla-maki
Copy link
Contributor

mikayla-maki commented Dec 19, 2024

Hmm, I think this is a good feature but I don't quite like the look of that menu yet, cc: @danilo-leal, @iamnbutler, how do you think we could incorporate this setting / this UI?

For reference, VSCode uses a button in the top right of the title bar, and that button opens up a menu with an overall layout menu:

Screenshot 2024-12-19 at 2 19 48 PM

@th0jensen
Copy link
Author

For reference, VSCode uses a button in the top right of the title bar, and that button opens up a menu with an overall layout menu

I think this makes sense too. It feels unnatural to have the layout control on the dock when the property pertains to the workspace itself- and changing the value does change the button's origin, which is horrible for UX:

If someone wants to change the layout on the current build, they'll have to chase the button around in order to reset or change the layout.

I have noticed that VS Code also has this in the Menu Bar -> View -> Layout (something like this)

@iamnbutler
Copy link
Member

While it looks really small, this is a pretty massive change from our standpoint, as all UI in zed thus-far has been designed and build from the perspective of sidebars always extending the full height of the window.

I'm not against us exploring it, but it'll probably have to wait until after the holidays before I can take the time to look into this and give proper feedback.

Thanks for the patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an option to allow the bottom dock to be full width under the left dock
3 participants