Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Diagnostics Panel - Improvements #39

Open
mehcode opened this issue Sep 16, 2017 · 13 comments
Open

Diagnostics Panel - Improvements #39

mehcode opened this issue Sep 16, 2017 · 13 comments

Comments

@mehcode
Copy link

mehcode commented Sep 16, 2017

I've talked a bit about this before in other places. I started an experiment for improving the linter's panel. Now that atom-ide-ui is the "official" solution I'd like to try and improve the UX of the panel in the same way.

Here are some thoughts. Numbered for later reference only.

  1. Add a search box in the top-right.

    • Free text searches against the description.
    • source:eslint searches against the source
    • is:error or is:warning searches against type
  2. Remove the table header. My opinion is that sorting messages by anything except file, line isn't very useful.

    • ctrl-click (or cmd-click) on a specific Source could populate the search box with source:<source>
  3. Use icons instead of words to communicate severity. On my screen, Warning is always cut off as Warni... and it looks a bit sloppy.

screenshot from 2017-09-15 18-04-25

  1. Group messages by file with section headers. This gives much more room in the line for the important bit, the message.

I have several more ideas but I'd like to focus on those 4 first. We can incrementally improve what's there by working on this one piece at a time.

Thoughts? If we can agree on the general direction I can start working on this.

@matthewwithanm
Copy link
Contributor

matthewwithanm commented Sep 16, 2017

Hey! I'm actually working on a lot of this as we speak (:

  • The search box is already there. It's just a text search across all of the fields so "eslint" will match the eslint messages and "error" will match the errors, etc.
  • We toyed with the idea of the table header but ultimately decided to keep it.
  • Yup, we've got icons!
  • Totally agree about the message being the important bit, but we decided to stick with the table for now because it's more space efficient vertically and the vertical space is generally at more of a premium.

There's a setting for you to opt in but I've still got a lot to go yet!

@damieng
Copy link

damieng commented Sep 16, 2017

  1. Love the idea and the filtering syntax proposed makes sense
  2. I like the table header - sometimes I sort by message because I want to tackle all the same type of problems together
  3. Agree with this
  4. Having a group by source or file is a cool idea but if you have filtering it has a little less value. Especially if the filter box can autocomplete, e.g. is: prompts error, warning etc.

@mehcode
Copy link
Author

mehcode commented Sep 19, 2017

Here's an example of linter-ui-plus's panel:

Ignoring the toolbar as I'm not a fan of it aside from the search.


There's a setting for you to opt in but I've still got a lot to go yet!

Nice! 👍

The search box is already there. It's just a text search across all of the fields so "eslint" will match the eslint messages and "error" will match the errors, etc.

Some suggestions based on what's there currently in preview:

  • Tokenize the search terms so foo bar matches foo message more message bar
  • Highlight matching text (primary font color perhaps)
  • Allow [flow] or source:flow to search only the source column. Some times I really just want to look at one source.

We toyed with the idea of the table header but ultimately decided to keep it.
I like the table header - sometimes I sort by message because I want to tackle all the same type of problems together

Some suggestions then from what I see on preview.

  • I see you've already merged the "File Name" and "Line Number" columns. Further merge with the "Path" column and name the column "Location". Further,
  • Don't bold on highlight. The visual jump is jarring.
  • Don't have a column header title for the "Type" column. You really just want that column to be an icon and the word "Type" is already too large.
  • Don't allow column resizing and auto-fit contents. I just want it to look clean at all times without having to "mess with" the column sizes.
  • Lastly the column border on the left of path isn't flush with the header.

Yup, we've got icons!

They don't seem to work at all when I enable preview. Perhaps known bug?

Having a group by source or file is a cool idea but if you have filtering it has a little less value. Especially if the filter box can autocomplete, e.g. is: prompts error, warning etc.
Totally agree about the message being the important bit, but we decided to stick with the table for now because it's more space efficient vertically and the vertical space is generally at more of a premium.

The value adds for grouping by file are:

  • Ability to collapse files. This can be useful to "collapse all" and only open one at a time when browsing.
  • Visually break up sections without needing to alternative background colors which looks dated.
  • Allowing more space for the message. I disagree that vertical space is at a premium. I don't want to see 14-20 error messages at once. To me, that's too much to digest. They all sort of run together. Bringing that down to 8-12 is much more manageable and allows for useful padding to allow separation.

@matthewwithanm Not sure what "Hide Feedback" is on the preview panel. Doesn't seem to do anything for me.


Some additional suggestions as @matthewwithanm seems to be tackling this UI+ for the panel.

  1. Instead of "Hide Errors" and "Hide Warnings" have a single "Show Only Errors" toggle button. From a user flow, this is all that's needed. As a user I either want to shift through all of my problems or only see my errors. Only seeing my warnings is not nearly as useful and should not be a button. Perhaps is:warning in the search box.

  2. Add a "Show Only Current File" toggle button (not checkbox as it is in current panel).

  3. Add a "Show Only Changed Files" toggle button. The idea is to only show errors/warnings from files that you've changed in Git (unstaged or staged). From talking to users of Linter this a workflow that is very common and done manually using a "show only current file" option in the panel.

  4. Require double click to "go to source" on panel. Single click should just select the line. I should then be able to use up/down keys to navigate the panel. Think of the tree view for precedence here.

  5. I would keep the toggle buttons on the far right (as they affect filtering really). The far left can have warning/error counts. These counts are still useful as I may have a filtered pool I wish to have an error/warning count of.


Sorry for the text wall. Just trying to communicate everything I'm thinking here. I was planning on doing a series of pull requests but it seems @matthewwithanm is on the job. Good work so far and let's make this panel amazing. 🥇

@matthewwithanm
Copy link
Contributor

Wow, thanks for all the feedback!

  • Tokenize the search terms so foo bar matches foo message more message bar

👍 yeah this definitely needs to happen

  • Highlight matching text (primary font color perhaps)

👍 sounds good

  • Allow [flow] or source:flow to search only the source column. Some times I really just want to look at one source.

this one i'm no so sure about. query languages like this aren't the most discoverable and i think it's pretty rare that a fulltext search for "flow" wouldn't accomplish the same thing.

Some suggestions then from what I see on preview.

  • I see you've already merged the "File Name" and "Line Number" columns. Further merge with the "Path" column and name the column "Location". Further,

These are intentionally separate so that you can easily control how much of the path you want to see while still keeping the filenames aligned. I'm planning on making the path column disabled by default to reduce clutter, but haven't gotten there yet.

  • Don't bold on highlight. The visual jump is jarring.

👍 This is in the cards; it's just using our default table styling right now. I need to talk to @jgebhardt and see if it makes more sense to change our default table styling or have this one be a one-off.

  • Don't have a column header title for the "Type" column. You really just want that column to be an icon and the word "Type" is already too large.

I actually floated the idea of not having any of the header text and reducing the header height 🤔…@karincurkowicz what are your thoughts on this?

  • Don't allow column resizing and auto-fit contents. I just want it to look clean at all times without having to "mess with" the column sizes.

I don't think we want to give this up but we could probably do a better job of getting initial sizes for some of the columns so it's less important.

  • Lastly the column border on the left of path isn't flush with the header.

Hm, this sounds like a bug but I'm not seeing it.

They don't seem to work at all when I enable preview. Perhaps known bug?

No! Not seeing this one either…

The value adds for grouping by file are:

Yeah, there are definitely tradeoffs. @karincurkowicz explored the grouped option but we didn't want to lock users into a particular grouping (like @damieng noted). For now we're going to go with the table but we can always revisit later.

@matthewwithanm Not sure what "Hide Feedback" is on the preview panel. Doesn't seem to do anything for me.

We'll be showing review comments in this table too, and this refers to that. When I'm done, the button won't be visible if there aren't any providers that support that message type. I probably should rename this to "Review."

  1. Instead of "Hide Errors" and "Hide Warnings" have a single "Show Only Errors" toggle button. From a user flow, this is all that's needed.

In the long term, we want this to be a home for all of the things you need to fix in your code. The first step for that supporting review comments but ultimately that'll expand to things like test failures too. So this part of the UI is looking forward to that 😊

  1. Add a "Show Only Current File" toggle button (not checkbox as it is in current panel).

👍

  1. Add a "Show Only Changed Files" toggle button. The idea is to only show errors/warnings from files that you've changed in Git (unstaged or staged). From talking to users of Linter this a workflow that is very common and done manually using a "show only current file" option in the panel.

😍 Love it!

  1. Require double click to "go to source" on panel. Single click should just select the line. I should then be able to use up/down keys to navigate the panel. Think of the tree view for precedence here.

This is our plan 😊. I'll be pushing the up/down key stuff later today probably. tbh, though, I've been questioning the double click thing lately and wondering whether we should keep single click as select + confirm…the thought being that there's no reason for a primarily mouse-driven user to click on an item other than to jump on it. Requiring double click to open is really only useful for people who want to use a mouse (to select) and then the keyboard to navigate, which is probably really just a manifestation of inadequate keyboard controls for setting focus.

Sorry for the text wall. Just trying to communicate everything I'm thinking here. I was planning on doing a series of pull requests but it seems @matthewwithanm is on the job. Good work so far and let's make this panel amazing. 🥇

No need to apologize! It seems like we're running along the same tracks here for most of this. I would love to get your help with this if we have a shared vision and can figure out a way to not step on each other's toes. ❤️

@mehcode
Copy link
Author

mehcode commented Sep 19, 2017

In the long term, we want this to be a home for all of the things you need to fix in your code. The first step for that supporting review comments but ultimately that'll expand to things like test failures too. So this part of the UI is looking forward to that

❤️

If you want to keep both an "hide errors" and "hide warnings" buttons I'd suggest switching them to look more like tabs with an "All" tab. Then you can move the global filtering controls (only file, only changed files, etc.) to the right of the search bar. The remaining controls would be type filtering.

Requiring double click to open is really only useful for people who want to use a mouse (to select) and then the keyboard to navigate, which is probably really just a manifestation of inadequate keyboard controls for setting focus.

Mostly agree. But I think we should operate like the tree view does for consistency.


Out of time to go into more but I'll post some screens of what I'm seeing in relation to the preview bugs on separate issues later.

@Arcanemagus
Copy link
Contributor

The thought being that there's no reason for a primarily mouse-driven user to click on an item other than to jump on it.

Keep the use case of selecting the text of a message in mind, especially right now when the url property isn't being handled, but even when it is there are providers that don't provide links to the documentation so being able to copy text to search for what the error could mean is extremely useful.

@karincurkowicz
Copy link

I actually floated the idea of not having any of the header text and reducing the header height.

@matthewwithanm and I discussed removing the "Type header", then we discussed removing all the headers, as all the content is generally distinguishable.

We came to the conclusion that removing one label would be confusing & inconsistent. While removing all labels would create problems for folks looking for an obvious location to sort the table. I think that while folks are learning their way around the new table interactions we keep the table header in place. We can further tune it based on future feedback & usage.

facebook-github-bot pushed a commit to facebookarchive/nuclide that referenced this issue Sep 21, 2017
Summary:
This adds support for keyboard navigation via the `core:move-*` commands, as well as a separate concept of "confirming" a selection. Currently, this is bound to a single click, however, this may change to double-click in the near future.

See facebookarchive/atom-ide-ui#39 for some discussion.

Reviewed By: jgebhardt

Differential Revision: D5866099

fbshipit-source-id: 97e6e26f7ec5446bf8f1fd7fc3dc89e987a91a25
hansonw pushed a commit that referenced this issue Sep 22, 2017
Summary:
This adds support for keyboard navigation via the `core:move-*` commands, as well as a separate concept of "confirming" a selection. Currently, this is bound to a single click, however, this may change to double-click in the near future.

See #39 for some discussion.

Reviewed By: jgebhardt

Differential Revision: D5866099

fbshipit-source-id: 97e6e26f7ec5446bf8f1fd7fc3dc89e987a91a25
@matthewwithanm
Copy link
Contributor

@mehcode Any interest on putting together a PR for some of the search stuff? I've got a ton of stuff on my plate and would love a hand! Say, maybe these?

Tokenize the search terms so foo bar matches foo message more message bar
Highlight matching text (primary font color perhaps)

(Maybe use bold for the matching text instead of changing the color?)

@matthewwithanm
Copy link
Contributor

(Fix on the way for the icons issue. Sorry about that!)

@mehcode
Copy link
Author

mehcode commented Oct 7, 2017

@matthewwithanm Swamped at the moment myself with my latest work project. I don't think I'll have a chance to do those until a week or two out. If they're still not done then, I'll take them.

@matthewwithanm
Copy link
Contributor

Awesome! lmk 💜

@mbylstra
Copy link

image
I'm liking the new design, but I'm really missing being able to see line numbers when 'current file only' is checked. I use vim bindings so it was easy to navigate to the code by typing (eg) 68G. Now, the only way to get to the error is by using the mouse, and then a further click is required to actually get the cursor to the line.

@Arcanemagus
Copy link
Contributor

@mbylstra See #101.

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

No branches or pull requests

6 participants