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

Align quickpick labels #12247

Closed
wants to merge 7 commits into from

Conversation

gbodeen
Copy link
Contributor

@gbodeen gbodeen commented Mar 1, 2023

What it does

This is an alternative to #12239.

I noticed a few flaws in the quickpick styling when searching for a file,

  • Some results have a generic icon image. For those results, the filename is horizontally offset and the path is vertically offset.
  • Some results have a long path name, which can entirely hide the file name. This MR proposes following the style of VSCode, which shows the complete filename, then at a reduced opacity as much of the path as will fit.

image

With the change:

image

Compare to VSCode:

image

A more perfect change would refactor the CSS (e.g. using BEM) to avoid the high levels of selector specificity in monaco-editor-core that lead to this. However, that would also have complex effects. This PR should have more narrowly determined consequences.

How to test

  1. Open the quickpick to search for a file (Ctrl + P). Search using various strings and observe the problems described above.
  2. On this branch, search and observe better formatting.

Review checklist

Reminder for reviewers

@gbodeen
Copy link
Contributor Author

gbodeen commented Mar 1, 2023

I'm going to address this problem in a different manner that also handles a few related problems.

@gbodeen gbodeen closed this Mar 1, 2023
@gbodeen gbodeen reopened this Mar 1, 2023
@gbodeen
Copy link
Contributor Author

gbodeen commented Mar 1, 2023

I see a problem with the underscores. I'll aim to fix that soon. fixed

@gbodeen
Copy link
Contributor Author

gbodeen commented Mar 1, 2023

OK, now this also fixes the debug quickpick similarly as in #12239 👍

image

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

All the listed scenarios, including those from #12239, look good to me.

@vince-fugnitto vince-fugnitto added monaco issues related to monaco ui/ux issues related to user interface / user experience labels Mar 2, 2023
@vince-fugnitto
Copy link
Member

@gbodeen I've noticed issues with other file-icon themes such as seti:

image

@colin-grant-work colin-grant-work self-requested a review March 2, 2023 19:51
@colin-grant-work colin-grant-work dismissed their stale review March 2, 2023 19:52

Pending fix of icon alignment issue

@gbodeen
Copy link
Contributor Author

gbodeen commented Mar 2, 2023

seti icon theme:

image

theia icon theme:

image

debug menu still good:

image

...so empirically this draft is looking better. I think it's also theoretically better, as the current approach is:

  • make the codicon default-file-icon pseudoelements have essentially all the same properties as the theia-file-icons-js file-icon pseudoelements
  • don't allow the wrong font & font-size to leak out of the codicon default-file-icon elements
  • still unset the flex property that was inherited from elsewhere and creating the debug problem

@FernandoAscencio
Copy link
Contributor

Found an issue with the check marks next to toggle commands.
Theia browser Microsoft Edge: 110.0.1587.57

alignmentBrowserEdge

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.

@gbodeen do you mind explaining the change from f41e71b? If we are to make the decision to remove the checkmark from isToggled commands it should probably be documented as a breaking change, and it should not be removed as part of the css but rather where we add the checkmark in the first place:

private getItemIconClasses(command: Command): string[] | undefined {
const toggledHandler = this.commandRegistry.getToggledHandler(command.id);
if (toggledHandler) {
return codiconArray('check');
}
return undefined;
}

@gbodeen
Copy link
Contributor Author

gbodeen commented Mar 7, 2023

@vince-fugnitto

do you mind explaining the change from f41e71b?

Perhaps I misunderstood. I thought the checkboxes were never supposed to be visible. My second draft, as discovered by Fernando above, had an unexpected side effect of making them visible under certain circumstances where they had not previously been visible.

What is the intended behavior? (I'll be able to touch this again later this week.)

@FernandoAscencio
Copy link
Contributor

I thought the checkboxes were never supposed to be visible.

Those are not checkboxes but check codicons that should appear when a toggle command is "on".
The issue is that they are not aligned with the rest of the text.

@gbodeen
Copy link
Contributor Author

gbodeen commented Mar 8, 2023

Interesting. I'd never seen them before and presumed they were only used in the code, not something that should be visible to users.

In any case, it now follows the same approach as above, applying all the same properties as used for icons in other areas of the code, and the result is well aligned:

image

msujew
msujew previously approved these changes Apr 17, 2023
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

The changes look good to me. Different icon themes and the checkmarks seem to layout correctly.

@vince-fugnitto Care to take another look as well?

@vince-fugnitto
Copy link
Member

@msujew I still noticed an issue and possibly others where the icons are not properly aligned.
For example:

term.mp4

@msujew msujew dismissed their stale review April 17, 2023 15:15

There are still other misaligned icons

@vince-fugnitto
Copy link
Member

Closing in favor of the updates from #12239.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco ui/ux issues related to user interface / user experience
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants