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

Keymaps: Keyboard shortcuts text is too small #7060

Merged
merged 1 commit into from
Feb 5, 2020

Conversation

Anasshahidd21
Copy link
Contributor

What it does

Issue ID: #7051
Fixed the sizing of the font, as well as the spacing of each column just so we can utilize the empty spaces in filling up other column's data and still look clean.

Signed-off-by: Muhammad Anas Shahid muhammad.shahid@ericsson.com

How to test


- Open keyboard shortcuts by going to File>Settings>Open Keyboard Shortcuts

- The font size of the context/when has been increased by a slight amount, and the columns are resized.

Review checklist

Reminder for reviewers

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I think we need to rethink the column widths better.
For instance, I do not expect that source is cutoff as so when I first open the widget with something like the explorer opened.

Screen Shot 2020-02-03 at 4 50 31 PM

Compare this with the implementation present in VS Code:

Screen Shot 2020-02-03 at 4 52 50 PM

We should ultimately prioritize:

  • that commands and keybindings are displayed entirely when possible
  • that the source is displayed properly (should not cutoff the header name)
  • that the context (since it is the largest) should be prioritized last

In any case each of these fields can be hovered to display the full content

}

.kb table .th-keybinding {
width: 20%;
Copy link
Member

Choose a reason for hiding this comment

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

I'd maybe go 15%-20%, some keybindings are not visible enough.
In general I think that commands and keybindings should be the main priority.

Other columns can have their content shortened and can be fully displayed by hovering over the cells.

For example, look at the following case:

Screen Shot 2020-02-03 at 4 46 28 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update:

  1. Increased widths for commands and keybindings
  2. Source is now displayed properly, the way it is done in VSCode
  3. The context is resized to a smaller size to give more priority to the other two important ones.

Ready for Review.

@vince-fugnitto vince-fugnitto added keybindings issues related to keybindings ui/ux issues related to user interface / user experience labels Feb 3, 2020
Issue ID: eclipse-theia#7051
Fixed the sizing of the font, as well as the spacing of each column just so we can utilize the empty spaces in filling up other column's data and still look clean.

Signed-off-by: Muhammad Anas Shahid <muhammad.shahid@ericsson.com>
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The latest changes look good to me, thank you!

@vince-fugnitto
Copy link
Member

I don't think others are interested in such a change so I'll go ahead and merge :)

@vince-fugnitto vince-fugnitto merged commit 9eea8ae into eclipse-theia:master Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keybindings issues related to keybindings ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants