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

ToolbarButton: Can't read "Pull origin with rebase" because of truncation #17388

Open
johanekhager opened this issue Sep 14, 2023 · 9 comments · May be fixed by #18095
Open

ToolbarButton: Can't read "Pull origin with rebase" because of truncation #17388

johanekhager opened this issue Sep 14, 2023 · 9 comments · May be fixed by #18095

Comments

@johanekhager
Copy link

johanekhager commented Sep 14, 2023

The problem

Toolbar buttons truncate important information

Toolbar buttons uses a fixed width of 230px today which makes important text truncate.

Today, push-pull-button will be "Pull origin with...". Would be far better if we don't truncate this button.

Screenshot 2023-09-14 at 08 39 17

Would make sense to either increase width to 280px or add a tooltip on truncated text.

Release version

Version 3.3.1 (arm64)

Operating system

MacOS

Steps to reproduce the behavior

No response

Log files

No response

Screenshots

No response

Additional context

Just change:

  .toolbar-button {
    &.push-pull-button {
      width: 230px;
    }
  }

  .toolbar-button {
    &.branch-toolbar-button {
      width: 230px;
    }
    &.revert-progress {
      width: 230px;
    }
  }

To:

  .toolbar-button {
    &.push-pull-button {
      width: 280px;
    }
  }

  .toolbar-button {
    &.branch-toolbar-button {
      width: 280px;
    }
    &.revert-progress {
      width: 280px;
    }
  }

Or add a tooltip. Or let me help you. <3

@tidy-dev
Copy link
Contributor

Just some notes:

  • We likely do not want to set the width to wider as the widths of these things have been determined based on smallest window constraints/zoom capabilities.
  • If a tooltip is added we need to make sure focusing the button with the keyboard also invokes the tooltip so that it is not only mouse users who can access the clipped content.

@johanekhager
Copy link
Author

Just some notes:

  • We likely do not want to set the width to wider as the widths of these things have been determined based on smallest window constraints/zoom capabilities.
  • If a tooltip is added we need to make sure focusing the button with the keyboard also invokes the tooltip so that it is not only mouse users who can access the clipped content.

Thanks for your comments.
I just tested and because we use min-width: 0 on .toolbar-button and it's parent it will truncate the text and adjust width to screen size. So going from 230px to 280px for example, wouldn't make much difference for smaller screen sizes.

230px, 200% zoom, 1024px screen size 280px, 200% zoom, 1024px screen size
Screenshot 2023-09-15 at 08 37 22 Screenshot 2023-09-15 at 08 36 50

But it will look better for larger screen sizes:

230px, 100% zoom, 1280px screen size 280px, 100% zoom, 1280px screen size
Screenshot 2023-09-14 at 08 39 17 Screenshot 2023-09-14 at 08 38 57

@tidy-dev
Copy link
Contributor

My concern is the clipping of the dropdown button on the smallest constraints. It is already clipped some. That said, maybe a place for a media query so larger screens can have this benefit.

Below is 280px, 200%, smallest window size.
image

@johanekhager
Copy link
Author

My concern is the clipping of the dropdown button on the smallest constraints. It is already clipped some. That said, maybe a place for a media query so larger screens can have this benefit.

Below is 280px, 200%, smallest window size. image

Ah, alright! Didn't test with the dropdown. If you add flex-shrink: 0 to .toolbar-dropdown-arrow-button that will do the trick here if I didn't miss something. 🎉

@tidy-dev
Copy link
Contributor

I will bring it to my team next team sync to make sure there isn't something I am missing. If not, I will label this under help wanted Issues marked as ideal for external contributors to open up to community contribution. :)

@tidy-dev
Copy link
Contributor

tidy-dev commented Sep 18, 2023

Team sync meeting clip notes: (will provide a more detailed response after further discussion)

  • preferred solution would implementing a flexible constrain system. Something like we have for the first toolbar button
    • See sidebarWidth in app-state.ts and updateResizableConstraints() in the app-store.ts.
    • This would apply to both the branch and pull/push/fetch button.
  • adding a tooltip is also a possibility, but currently the one on the branch dropdown is not accessible for keyboard users so we could not just copy what is done there.
  • media query could be ideal for the small screens (typical responsive design: allow wrapping, removing unneeded stuff, moving stuff around) in addition to flexible constrain system.

@jpedroso
Copy link

For a long time, what bothered me the most was not being able to read the branch name past feature/… in the branch dropdown button once the PR is open, exactly like shown in the OP. There too, I had to hover and wait for the tooltip to appear.

So, I made the branch dropdown button adjustable using Resizable. It works like this.

2023-09-22-GitHub.Desktop-dev-000008.mp4

This was almost 2 years ago. I brought the changes up to date with development and surprisingly there are no conflicts, and works the same except it’s missing the use of constraint() in app-store.ts loadInitialState().

Any interest in seeing a PR?

@tidy-dev
Copy link
Contributor

@jpedroso Thank you for your work on this and requesting in this issue and the branch width issue. I chatted with the team and we are interested in seeing a PR. Tho if we are going to have the branches toolbar resizable, we would also like to see the pull/push/fetch button have a same applied to it as it has it's own size limitations as noted in this issue.

Things that will need to be tested:

  • Each toolbar button should have minimum width (I believe you have address that in the other thread)
  • The toolbar buttons need to make use of contraint such that when zooming in the minimum window the toolbar buttons do not clip. (similarly to how they shrink to fit when a user resizes from a large to smaller window)
  • Ensure the "Empty" states of the dropdown still look good. Currently the branch dropdown tabs have imagery that are sized specifically to the current set width.

I would also like to note that we have limited bandwidth for reviewing PRs so I can not promise quick responses to the pr being opened.

@jpedroso
Copy link

@tidy-dev pr #18095 submitted for this. I believe I address all aspects you mention, including making the push/pull button resizable too, and dealing with empty states accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants