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 width setting for the file finder #18682

Merged
merged 6 commits into from
Nov 16, 2024

Conversation

isaacdonaldson
Copy link
Contributor

@isaacdonaldson isaacdonaldson commented Oct 3, 2024

This PR adds the ability to adjust the width of the file finder popup. I found when searching my projects the default width was not always wide enough and there was no option to change it.

It allows values small, medium (default), large, xlarge, and full

Release Notes:

  • Added a setting to adjust the width of the file finder modal

Example Setting:

  "file_finder": {
    "modal_width": "medium"
  },

Screenshots can be found in the comments below.

Copy link

cla-bot bot commented Oct 3, 2024

We require contributors to sign our Contributor License Agreement, and we don't have @isaacdonaldson on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

@isaacdonaldson
Copy link
Contributor Author

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Oct 3, 2024
Copy link

cla-bot bot commented Oct 3, 2024

The cla-bot has been summoned, and re-checked this pull request!

@thorbenk
Copy link
Contributor

thorbenk commented Oct 3, 2024

See

@isaacdonaldson
Copy link
Contributor Author

See #16018

Ah my apologies, I looked for a past issue and couldn't find one, so I missed this one. This PR would solve the issue of not being able to configure the width of the file finder that is mentioned there too.

@danilo-leal
Copy link
Contributor

danilo-leal commented Oct 3, 2024

Very cool—thanks for the PR! I have a feeling it could make sense to tokenize the width value as opposed to being just an arbitrary number. We do already have compact, comfortable, and default as tokens that represent density (see #18078), which we could possibly be used here, too, but I'm not sure yet.

Also, this doesn't necessarily mean that we wouldn't continue to allow passing arbitrary numbers—it's just so that there's a set of pre-established defaults that the whole app follows when it comes to UI config.

cc @iamnbutler for further thoughts!

@maxdeviant
Copy link
Member

I think we may want to make the default behavior to have the modal size itself more intelligently to fit the available space rather than jumping to making the width configurable.

@isaacdonaldson
Copy link
Contributor Author

isaacdonaldson commented Oct 3, 2024

I think we may want to make the default behavior to have the modal size itself more intelligently to fit the available space rather than jumping to making the width configurable.

@maxdeviant Yeah I agree. I changed it to have 4 width settings: "small" | "medium" | "large" | "xlarge" where "medium" is the default. The settings correspond to how much padding is on the side of the popup ("small" will have the largest padding). There is also a minimum width for the window so it will work fine on very small screens.

Screenshots:

Small:
Screenshot 2024-10-03 at 3 51 21 PM

Medium:
Screenshot 2024-10-03 at 3 54 12 PM

Large:
Screenshot 2024-10-03 at 3 52 34 PM

XLarge:
Screenshot 2024-10-03 at 3 52 55 PM

Here is the behaviour when the window is shrunk with the "xlarge" setting:

large_shrink.mov

And when it hits the minimum size:

small_shrink.mov

And finally, what the "xlarge" setting looks like when in a small window:
Screenshot 2024-10-03 at 4 02 45 PM

@alejandrovrojas
Copy link

I changed it to have 4 width settings: "small" | "medium" | "large" | "xlarge" where "medium" is the default.

@isaacdonaldson This could be a good opportunity to implement full as well!

project-search-b

@isaacdonaldson
Copy link
Contributor Author

@alejandrovrojas - I added the "full" option for a fullscreen width finder window.

Maybe someone from Zed can let me know if there are any desired changes, or if there is anything holding this PR up?

@thorbenk
Copy link
Contributor

I tried this branch and I like it! It solves my problem #16018

Thanks @isaacdonaldson, I hope this gets in 😄

@protoEvangelion
Copy link

@maxdeviant @ConradIrwin @mrnugget I'll literally switch from VSCode to Zed over this issue. It's been 3 years & they are still debating on how to allow users to override width: microsoft/vscode#117862
Is there anything else that needs to be done on this PR to push it through?

@SomeoneToIgnore SomeoneToIgnore self-assigned this Nov 16, 2024
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

This actually looks good, not sure why it got stalled.

Can we rebase and, optionally, add an entry to https://github.com/zed-industries/zed/blob/516f7b364213b2bbc1a658295df96443aa885ca0/docs/src/configuring-zed.md ?

And we're good to merge right after.

Edit: + Danilo's review comment is needed before the merge.

@isaacdonaldson
Copy link
Contributor Author

@SomeoneToIgnore - Sounds good, easy changes

Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

Thank you!

@SomeoneToIgnore SomeoneToIgnore merged commit 31566cb into zed-industries:main Nov 16, 2024
12 checks passed
danilo-leal added a commit that referenced this pull request Nov 18, 2024
Follow up to: #18682

This PR tweaks the setting value, so it's clear we're referring to
`max-width`, meaning the width will change up to a specific value
depending on the available window size. Then, it also makes `Small` the
default value, which, in practice, makes the modal size the same as it
was before the original PR linked above.

Release Notes:

- N/A

---------

Co-authored-by: Kirill Bulatov <[email protected]>
@dangh
Copy link

dangh commented Nov 19, 2024

It's might be too late to comment on this but I think computing the modal width by subtracting the paddings is not intuitive as it makes the modal "small" compare to the window, not the screen. On my mac, "small" modal is to wide on full screen width (takes too much of screen real estate and too wide compare to the command palette), and "medium" too small on half screen width (my paths are cut-off too much), which is very confusing.

Generally we want to define the minimum width of the modal so it can show long paths. If the modal is too wide for the small window, we shrink the modal so it can fit. An arbitrary number can help fine-tuned the setup for everyone that use more than one screens.

Also a nitpicking but I would add small x-padding to the "full" modal as it has shadow and rounded corners, it would looks better this way.

@danilo-leal may I have your input on this?

@xzbdmw
Copy link
Contributor

xzbdmw commented Nov 29, 2024

we can show full content when hovering or just selecting over long strings as JetBrains does:

iShot_2024-11-30_03.26.17.mp4

This logic is applied consistently throughout the entire IDE.

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.

9 participants