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

refactor: improve styles in shortcut hint #4588

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

Conversation

nicodh
Copy link
Member

@nicodh nicodh commented Jan 31, 2025

  • add alternative shortcut to open shortcut hints

Before:

image

After:

image

#skip-changelog

- add alternative shortcut to open shortcut hints
@nicodh
Copy link
Member Author

nicodh commented Jan 31, 2025

Of course the dialog now needs much more space, what can be considered worse...

Copy link
Collaborator

@WofWca WofWca left a comment

Choose a reason for hiding this comment

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

Looks much better on the screenshot, but on not-super-tall screens it's really weird.

2025-01-31-3hUo8q06sz.mp4

I don't think it should be horizontally scrollable.

@nicodh nicodh requested a review from WofWca February 2, 2025 11:56
@WofWca
Copy link
Collaborator

WofWca commented Feb 3, 2025

TBH I liked the original screenshot more.
IDK about three columns. Seems too much. And "Force Refresh Network" takes too much space IMO. I'd keep the layout, but only fix the spacing and wrapping.
This what it looks like now:

image

@nicodh
Copy link
Member Author

nicodh commented Feb 3, 2025

IDK about three columns. Seems too much.

Well otherwise you have a narrow window but have to scroll a lot. Not what I would expect if I have a wide enough screen.

@nicodh
Copy link
Member Author

nicodh commented Feb 3, 2025

This what it looks like now:

Looks weird. So you have a screen with less than 760px height, on the other hand no scrollbar?

@WofWca
Copy link
Collaborator

WofWca commented Feb 3, 2025

Looks weird. So you have a screen with less than 760px height, on the other hand no scrollbar?

No no, there is a scrollbar. I scrolled it to bottom, it's fine in this regard.

@nicodh
Copy link
Member Author

nicodh commented Feb 3, 2025

So I reduced the paddings and margins so that scrolling with 2 columns is less often used

Copy link
Collaborator

@WofWca WofWca left a comment

Choose a reason for hiding this comment

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

On 0.6x zoom there is always 1 column, no matter how wide the window is:

image

Comment on lines +65 to +66
padding-left: 10px;
padding-bottom: 10px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
padding-left: 10px;
padding-bottom: 10px;
justify-content: space-evenly;
padding-left: 10px;
padding-bottom: 10px;

Otherwise when there is one column it's kinda off IMO.

image

}
.keyboard-hint-dialog-body {
display: flex;
flex-wrap: wrap;
flex-direction: column;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't you think the column order is better? To me it seems intuitive to read it top-to-bottom. And there are no sections that are extra wide:

image

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.

2 participants