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 in-editor peek views (test, problems, references, etc.) UX #130516

Closed
miguelsolorio opened this issue Aug 10, 2021 · 20 comments
Closed

Align in-editor peek views (test, problems, references, etc.) UX #130516

miguelsolorio opened this issue Aug 10, 2021 · 20 comments
Assignees
Labels
error-list Problems view insiders-released Patch has been released in VS Code Insiders under-discussion Issue is under discussion for relevance, priority, approach ux User experience issues

Comments

@miguelsolorio
Copy link
Contributor

miguelsolorio commented Aug 10, 2021

Summary

This issue aims to capture the consolidation of the test peek view + problems "go to problem" view. The problems view has UI to view the context of the failures, similar to a peek definition, while the problems view has limited UI and causes users to navigate from the error to the problems panel to see the next set of problems/warnings.

Test failures

CleanShot 2021-08-10 at 12 48 21@2x

Problems view

CleanShot 2021-08-10 at 12 50 21@2x

Concepts

One idea is to combine the test UI with problems:

  • Add toolbar options for navigating and toggling the side view inside of the peek view
  • Provide inline actions for problems where relevant (quick fix)
  • Update colors to add visual refresh to peek view

CleanShot 2021-08-10 at 12 57 25@2x

cc @connor4312 @sandy081

@miguelsolorio miguelsolorio added ux User experience issues error-list Problems view labels Aug 10, 2021
@miguelsolorio miguelsolorio added this to the August 2021 milestone Aug 10, 2021
@miguelsolorio miguelsolorio self-assigned this Aug 10, 2021
@connor4312
Copy link
Member

I like the visual refresh. In previous discussions in my 1:1's with Kai I was an advocate for the combination (I think "alignment" might be a better word, not sure how much reuse there would be on a code level) to bring a tree or list view into the problems peek. I like having everything in one place and not have to look in the sidebar or panel when I'm going through issues.

@miguelsolorio
Copy link
Contributor Author

Some feedback we saw in our ux sync today:

  • The arrows should only appear when the side panel is not visible as that is primary way to navigate
  • We should align this work with our peek views as well (references)
  • Would expect for these to work similar to references where you can navigate to other tests
  • We should persist the side panel per each area (references, test, problems, etc.)

And below is an example of aligning the UI in the peek view with this:

CleanShot 2021-08-11 at 15 17 05@2x

@miguelsolorio miguelsolorio changed the title Align in-editor test failure and problems UX Align in-editor peek views (test, problems, references, etc.) UX Aug 11, 2021
@sandy081
Copy link
Member

So we are trying to bring the items explorer into the peek view that can be toggle-able? If so, Problem peek will have all problems from the file. How about test failure peek? Does it contain all test failures or history of test failures?

@miguelsolorio
Copy link
Contributor Author

Yes, the concept is that all peek views have the same building blocks (navigation, panel, etc.). For problems, showing all errors and warnings for what we show in the panel is what I would expect. In tests, it shows the history of the tests. So they are slightly different but still similar and I think that's ok. References shows all files where there are references to those items, so that's also slightly different. But the main blocks remain the same in terms of layout.

@byehack
Copy link

byehack commented Aug 18, 2021

  • support up/down key on peek view panel focused.

@miguelsolorio
Copy link
Contributor Author

@byehack can you elaborate? up/down keys already work on the peek view panel

@byehack
Copy link

byehack commented Aug 18, 2021

@byehack can you elaborate? up/down keys already work on the peek view panel

Rec.0029.mp4

stable v1.59.0

@miguelsolorio
Copy link
Contributor Author

@byehack the proposal here would be to bring the panel to the warning/error peek views so you will be able to navigate via up/down

@byehack
Copy link

byehack commented Aug 18, 2021

@byehack the proposal here would be to bring the panel to the warning/error peek views so you will be able to navigate via up/down

yes. and because of you are aligning all of peek views, i just would notice that the up/down keys supports well when side panel is not visible

@miguelsolorio
Copy link
Contributor Author

@byehack when the panel is not visible you will need to navigate to the up/down icons in the peek view or use the keyboard shortcut (F8):

CleanShot 2021-08-18 at 12 37 33@2x

@byehack
Copy link

byehack commented Aug 18, 2021

i am in v1.59.0 and default keyboard shortcut is Alt+F8/Shift+Alt+F8:
image

and Up/Down keys don't do anything. isn't it better to set it to arrow keys or should i manually change the default key bindings?

Edit: i noticed F8/Shift+F8 goes to next/previous problem in all files. (these keybindings can be remain as well as now.)

but below keybindings can be replace by Up/Down keys on peek view focuesed. (and even we can have them when peek view unfocused.)
image

@sandy081
Copy link
Member

sandy081 commented Aug 19, 2021

For problems, showing all errors and warnings for what we show in the panel is what I would expect.

Is it all or only those associated to the opened file? IMO showing only those for the given file makes sense.

@jrieken
Copy link
Member

jrieken commented Aug 31, 2021

I a fan of the updated colors and styles but I am very skeptical about showing a list of errors inside the error peek. In fact, I find the test fail/pass history annoying and useless. What's the value in knowing that a test passed before it failed? I would rather not have it and reduce clutter - which I now fear is being added into the peek error widget too.

The idea of the F8 error peek was to provide a stable view of the error hover not to duplicate or relocate the markers panel. The F8 gesture can than be used to "visit" all errors and warnings.

@jrieken
Copy link
Member

jrieken commented Aug 31, 2021

linking #73111 which is about adding a command to open the references view from the peek title

@miguelsolorio
Copy link
Contributor Author

miguelsolorio commented Aug 31, 2021

I think there are some users that would like a list of the rest of the problems (like myself) and others that don't. When looking through a list of errors, you have to move your eyes from the middle of the screen and then back down to the panel to scan the rest of the errors, and that is the benefit of the list. That's why we are wanting to push for having a toggle that would allow both groups to be satisfied. If people really didn't want to toggle this then a setting could hide it completely.

I think the a separate discussion is what the content inside of the list should be. In almost all other cases we show the current state/items and in the test explorer it is a history, which some have mentioned is a bit odd.

@miguelsolorio
Copy link
Contributor Author

Design explorations are done and assigning to the other area owners

@connor4312
Copy link
Member

Thanks for your great work on this Miguel 🙂

@jrieken
Copy link
Member

jrieken commented Sep 7, 2021

I think there are some users that would like a list of the rest of the problems (like myself) and others that don't.

That's making error peek an absolute outlier - all peeks show extra information about the "thing" its arrow is pointing at. Like, references of the symbol being pointed at, failures of the test being in etc. The same is true for squiggles: its the whole diagnostic for the current squiggle (message, source, related etc). Showing any message under a squiggle is just confusing because the message is out of context, like peek points at foo.bar() and its messages says cannot resolve console.lock()

I do understand the desire for good overview of errors and rapid navigation (that's why F8 is cross file) but peek isn't the right platform for that. We are also not discussing that breakpoint peek should include a list of all breakpoints or that quick-diff peek shows all changes in all files (they don't duplicate panes/views but show contextual information)

@sandy081
Copy link
Member

sandy081 commented Sep 7, 2021

Agreed with @jrieken opinions. I feel it's not worth duplicating problems view into peek view for the value it brings in. It will cause inconsistencies between these two views and could possibly lead to port all features to the list. I understand about consistency between peeks, but it adds noise and looses simplicity.

@sandy081 sandy081 added the under-discussion Issue is under discussion for relevance, priority, approach label Sep 7, 2021
@miguelsolorio
Copy link
Contributor Author

miguelsolorio commented Sep 8, 2021

Brought this up in the ux sync today and the consensus was that even though having richer navigation in the go to error/warning peeks, it ends up taking away from the "simplicity" of quickly looking at more information.

There was also discussion around how the Test peek has the history panel open by default is a bitt odd/unclear since it behaves differently from our other peek views. It was suggested to have an action in the toolbar to toggle the panel instead:

CleanShot 2021-09-08 at 14 51 09@2x

Given that, @connor4312 up to you if you want to keep this issue open for the test view or if you'd like a separate issue for that.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
error-list Problems view insiders-released Patch has been released in VS Code Insiders under-discussion Issue is under discussion for relevance, priority, approach ux User experience issues
Projects
None yet
Development

No branches or pull requests

6 participants
@jrieken @connor4312 @sandy081 @byehack @miguelsolorio and others