Every milestone, we will ask Product Designers and Engineers to volunteer to partner in making self-directed usability improvements. It is an opportunity to fix the things that have been bugging you or that you've heard from users without worrying about prioritization.
Self-Directed: There are no restrictions on where in the product the pair can make improvements. The goal is to empower the pair to focus on usability improvements that they personally want to see fixed in a product that they use themselves almost every day.
No restrictions on product area: The pair is not required to work within product areas owned by their own stage groups.
No restrictions on pairings: The Product Designer and Engineer pair do not need to be from the same stage group. This is a voluntary initiative.
Work in MRs, not issues: Both the Product Designer and the Engineer should work directly in MRs to make changes. For the Product Designer, these MRs will likely be focused on less complex usability issues that the pair identifies, such as documentation, minor UI polish, or UI text changes.
Length of rotation: The length of the pairing will be 1-3 milestones, depending on what the pairing believes is appropriate for them. This means that multiple groups could be working on Beautifying our UI in parallel.
Prioritization: The Product Designer and Engineer will inform their stage group team of their involvement in the initiative, so that they can make time for it during milestone planning.
@iamphill Would it be possible to add a filter just on merge requests? So we could default to showing specific things but give users the option to see all activity? (replacing the history/comments only dropdown)
@iamphill Yes! And I don't know why I keep talking about default; it would be a way easier transition to just keep the default to All and let users select what they want to see
So by default it might look something like this (or however the current select dropdown looks):
And then if you filter out some things, it could look like this:
Questions/notes:
What are your thoughts on putting this behind a feature flag? Or is that a huge additional effort?
The Apply filter button was just for the prototype testing; we could do whatever we currently do for applying selections
I don't know if these actions should be ordered by importance (in our opinion), alphabetically, or something else
I don't know if I would use it (mainly because I don't use a filter anyways )
@iamphill Interesting! Why would you not use it? Do you find all the activity entries useful? What filter do you not use- the comments/history only one?
Curious: Would these filters also apply to Notes on the Changes tab, though?
I think the filter by author of comment could be very useful to tune out the noise of multiple reviewers at once and being able to focus on the feedback of ONE reviewer at a time would be awesome.
Curious: Would these filters also apply to Notes on the Changes tab, though?
@andr3 Good question! It looks like that's how our current history/comment filter works, so I guess we'd keep it that way. Do you see any problems with that?
I think the filter by author of comment could be very useful to tune out the noise of multiple reviewers at once and being able to focus on the feedback of ONE reviewer at a time would be awesome.
Agreed! That was also part of the MR restructure prototype:
@annabeldunstone not really, but I'd recommend that somehow we keep an indicator visible that "content is being filtered" while seeing the Changes tab (which I don't think happens today).
@annabeldunstone I agree that consolidating some of these things down makes a lot of sense so there aren't so many filters. I think we could probably group these even tighter as the consolidated list still feels a bit long.
Maybe an approach would be what actions are probably infrequent (dates, time tracking, lock status, others?) and group those up somehow. I can try to spend some time thinking about fewer groupings if you think that would help, but I agree with your overall premise that fewer is better here.
Thanks for the ping and sharing the epic here. There is a lot going on in this issue so I will turn notifications off, however, please keep me in the loop during filter changes implementation.
I'm looking at consolidation at a different level, but it could still overlap.
I've always wondered if we should change the styling of the review comments to match normal comments, but just add a badge to indicate it is a review comment
Wondering if a badge would be useful or if it would add to the general chaos (that's too strong a word but I can't think of another one) of the activity.
We could look into it though. Or a user filter that you can only filter by users who have actually left a review
Or group all review comments into a collapsible section
I'm not sure about the green for approved It does match the checkmark in the sidebar, and the Approved text in the MR list view, so maybe it'll work here too
@annabeldunstone Argh. I misremembered. The flavor of Markdown in GitLab-the-product supports the display of hex color chips … but the version we use in the docs site does not.
Screenshot it is, then, with some accompanying text.
@annabeldunstone If we're using color to reinforce meaning in system notes, I want to call those meanings out in the system-notes docs. I was hoping I could use the hex numbers of each of the colors you mentioned in the table above to display colors without needing a screencap, but the flavor of Markdown we use in the docs doesn't support color chips. A screencap showing a few system notes of different colors, plus accompanying text, would be good on this page.
An idea, maybe improve the existing charts used in our dashboards (CI/CD Analytics, Insights, MR analytics, etc..).
The UI could be improved: colors, shades etc
Functionally some improvement could be done in aggregating some data, adding some filters, etc.
Not an expert here but lot of chart framework could fit: chart.js, Fusioncharts etc..
Another idea could be to improve grid view of certain pages. Like Environnement page. You need to open each environment to get some information. Having a data grid could help the global UX of the page.
All SUS-impacting issues need to have a proper severity label set.
Please add a severity label, remove the automation:ux-missing-labels label, and then reply to this comment briefly explaining your reasoning for providing this severity.
If you are not the DRI for this area and would like help determining the best severity, please @ the appropriate person for assistance.
Maybe not for this effort, but I wonder if at some point we should refactor that dropdown into a modal. The dropdown just doesn't look nice or work well in my opinion
I like the idea of also moving it into the Related merge requests widget:
But that would require the widget to always exist. And the button would behave very differently than the other similar buttons (unless we also allowed users to manually link an MR, similar to linking issues).
With saved replies rolling out - how would this work? Maybe for saved replies we could create a button inside of preferences that automatically creates these saved replies?
The thinking behind it was making specific types of comments non-blocking (so if you add praise to a line of code, it wouldn't need to be resolved). I don't think that's been tested though so probably not worth doing
I think Saved replies is taking care of the entire UI portion though
@iamphill They did, but we made the system notes smaller in this MR
I was wondering if we should make the highlighted ones back to the original size to help them stand out even more, but it might look a little buggy that way
I'm not sure if there's a bigger design/vision happening with that or if it was just done to save some space and make the comment avatars stand out more @seggenberger do you know?
I noticed in the screenshot in #387070 (comment 1229191914) that there is a system note for X reviewed, is this something we want? I think if we did this we could even make a filter for 'See activity since last review' which could be useful I would think
@annabeldunstone possibly. My workload is pretty intense being part of the early stages of groupproduct analytics (I'm wearing many hats…). I've had trouble trying to allocate time to this improvement as much as I really really want to get it in . I hold out hope I can work on it within the next two milestones.
Has there ever been a discussion or proposal to help navigate within the Overview tab?
When the description is VERY long and I want to jump to the Widgets section... How can I do that without having to scroll scroll scroll my life away? :-)
Essentially it could be simple small icons as "skip links" in the sticky header.
@andr3 There's a couple thoughts about this related to the restructuring effort and potentially as part of #389441 (closed).
I think ultimately the goal is to really re-think of how we show that information when it needs to be actioned, but get it out of the way otherwise.
Maybe there's something we could do in the interim, but I think both of the efforts for restructuring and potentially the reports tab are the long term direction.
@phikai good point! Yeah, not sure if it's worth dedicating effort in the interim. That Overview tab separated from Comments and having the Widget on top definitely would solve this, on a first quick look.
Do we need all the information we currently have in it still? Is there anything we can remove to make it less 'full' of information?
I don't know the full reason behind it currently but doesn't Pipeline #NUMBER failed 1 day ago sound better? Do I need to know its a merge result pipeline?
Should the main Failed icon match the rest of the widgets style?
The artifacts dropdown - is it even useful here? For the GitLab project is pretty large, so maybe this is just a us problem.
Should the Deploy to... be a seperate 'widget' all together?
@annabeldunstone I don't think it's weird that everything is left aligned, but there's some spacing inconsistencies in there that I think could be improved. The failed badge also isn't part of the widget design and should align with the widget specs.
TBH the design of the pipeline itself is something that I'd love to work on updating, but assuming that's out of scope here as that would be a decent effort by itself.
TBH the design of the pipeline itself is something that I'd love to work on updating
Yes please
@jeldergl Probably out of scope though, you're right. I didn't see a nice new icon for Failed pipeline (like the other widgets) so I hacked one together for the proposal in #387070 (comment 1326433610). Is there already an existing one?
Maybe the full mini-graph & download button should go in an expanded section instead of the Deploy stuff:
@annabeldunstone If this data goes into an expandable section, I would want that section to be expanded by default. If you know how your pipelines are set up, the stages status can already provide valuable information. (and it would save a click when navigating to the jobs)
@annabeldunstone I think the the design you proposed in your first comment is a nice improvement to make it look like other widgets. It doesn't seem to fundamentally change anything or remove any elements from the page.
I think all of the rest of the ideas in here are interesting and worth exploring, but probably a larger effort.
@iamphill Since we don't have enough info yet to move the deploy information, how about this:
Notes
Everything that currently exists in the widget is still there
Expanded by default
We can use the new icons for success/fail/warning (for the main pipeline status), and use our existing icons for the other states (pending/cancel/pause/running/etc)
I wonder if we could collapse it if the pipeline passes?
@iamphill Yeah that would be a nice addition. What would we do with the environment section though? Only collapse if the pipeline is passing and there are no environments? (or work on this once the environments have been moved elsewhere?)
@jeldergl Does that mean we'd use the universal fail and success icons for the pipeline status when it fails/succeeds, but continue to use the original icons for the other states (pending/running/paused/etc)?
@iamphill I just had another super fun idea we could try out and put behind a feature flag to test internally!
Review rounds lite
Sidebar
With the introduction of this new sidebar state, we should aim to keep all reviewers in the sidebar (no more unassigning yourself when you're "done")
When a user is assigned as a reviewer, any comment/review/approval they leave on that MR will affect the sidebar.
If Elroy Shaden leaves a review or comment, the sidebar will be updated to show the new reviewed but not approved status.
If another user who is not assigned as reviewer leaves a comment or review, nothing changes in the sidebar
This will help all subsequent reviewers get an immediate overview of who has reviewed, who is next, etc etc.
Merge requests badge
If you are assigned as a reviewer and you comment/review/approve that MR (therefore updating the sidebar), the badge counter will decrease by 1
The Review requests badge counter will only show MRs you need to act on (note: this doesn't touch/affect anything to do with your assigned merge requests)
Review requests list view
Same layout with the tabs etc, but the counter shows only MRs you have not reviewed. MRs that you have reviewed (commented/approved/reviewed) are grayed out (like "done" todos) and have a badge or something to show that you've reviewed them. Maybe they also go to the bottom of the list. They are still clickable (again, like todods). If you are asked to re-review, the MR will show up in the regular style again.
For example, let's say I've already reviewed or commented on these MRs that I am assigned as reviewer:
Current
Proposal
Roll out
We could enable this for all GitLab team members, making sure to update workflow documentation and spread awareness via Slack etc. None of this workflow will work if users unassign themselves after their review.
I think it makes sense! Its something we really need to figure out and soon, especially after attention requests not happening.
The main change from what I can see is we need to update the sidebar to add the re-request a review button when the user leaves a comment, not just a batch comments review.
The rest of the stuff, I think, should be relatively straight forward after that is done
@mnearents We're planning on working on this as part of Beautifying our UI to test it out internally. I think it'll end up solving quite a few of the same problems as the previous Attention requests feature.
This doesn't replace any of groupcode review's planned work for a full Review rounds experience; it's just a series of small enhancements that will ideally improve our internal workflow while providing real, useful feedback from team members.
When the MR is ready to be merged I'll open a feedback issue and respond to feedback as needed. We'll also updated the code review guidelines, announce (probably multiple times) on Slack, and add to engineering week-in-review to make sure as many people as possible know about the workflow change.
@aqualls Putting this on your radar! I'll open an MR to update the code review guidelines here but we will likely also need to update documentation too. This will all be behind a feature flag to test internally.
This hasn't been developed, but in broad strokes the main doc changes will likely be:
The MR reviewer badge counter will reflect MRs you have not yet reviewed
"My review requests" list will have a "done" state (shown in screenshots above). This just means you are assigned-as-a-reviewer && commented/reviewed-but-not-approved
No rush here; it will trickle into next milestone. Let me know if you'd like me to write the first draft!
@annabeldunstone Ok, now that I've been here a little longer and have a better grasp on things, I think a lot of this makes sense. Especially
The Review requests badge counter will only show MRs you need to act on
The parts I'm not sure about are:
If Elroy Shaden leaves a review or comment, the sidebar will be updated to show the new reviewed but not approved status.
Is this the dotted circle? This could get tricky. A reviewer leaving a comment could be more for understanding or to let someone know they'll take a look, or contributing to an existing conversation not related to the review at hand, etc. I could see a "blocked" status living there if the reviewer specifically left a blocking comment, or submitted an official review without approving, but I'm wondering if this is something that needs validation.
And I'm also unsure about showing grayed out MRs:
At what point would grayed out reviews move out of my list? Or would they always live there? That is a different behavior from todos which could be confusing. If I click "Needs review" and I see MRs that don't need review, whether they're grayed out or not, that isn't necessarily expected. I think the boring solution could be to introduce new filters, rather than new behaviors on the page.
Right now it's Reviewer = Me. But that could include past or present. So maybe the filters for this page should be Reviewer = Me Status = Incomplete, or simplify it to Needs review from = Me. A separate filter: Reviewer = Me would show everything if I was interested in that (no need for graying them out). When I click approve, the MR is no longer in the "Needs review from = me" state, but in the "Reviewer = me" state. Before I click approve, it's both.
I wanted to add to my above comment that I think a change like this may be too big for a beautifying our UI effort, and we could give it more thought and focus in our current review rounds research. I think big changes like this create too much of a moving target for Code Review UX work and I think some of the changes proposed here that seem small, are actually really big/complex (reviewed but not approved status, for example) and deserve a DRI level of attention. @annabeldunstone@andyvolpe@phikai@pedroms
I think a change like this may be too big for a beautifying our UI effort
@mnearents I don't agree, but I'll try to address the main themes/concerns here.
Reviewed but not approved
I think some of the changes proposed here that seem small, are actually really big/complex (reviewed but not approved status, for example)
This feature is already in the product. If you're assigned as a reviewer and submit a review without approving (and keep yourself assigned!), you'll see it.
This was also extended a bit to include more scenarios for when the re-request review feature shows up. I think it makes sense to cast the net even wider, and show both the Reviewed but not approved state along with the Re-request review button whenever a reviewer has commented.
A reviewer leaving a comment could be more for understanding or to let someone know they'll take a look, or contributing to an existing conversation not related to the review at hand, etc
In any of these scenarios, the reviewer or author can use the Re-request review feature. Doing so will remove the approval status (whether that's Approved or Reviewed but not approved) and re-add it to your list of review requests.
My review requests
At what point would grayed out reviews move out of my list? Or would they always live there? That is a different behavior from todos which could be confusing. If I click "Needs review" and I see MRs that don't need review, whether they're grayed out or not, that isn't necessarily expected. I think the boring solution could be to introduce new filters, rather than new behaviors on the page.
I'm honestly not sure what the boring solution is here. It seemed more complicated to add additional filtering/tabs, so for this iteration I proposed simply restyling the MRs you've reviewed. Once implemented, we could work on a more robust filtering system (providing the overall experience makes sense).
The MRs would live there until the MR is merged or closed.
Overall
Is this perfect? Absolutely not. I know there are problems with the proposal but I'd like to continue our bias for action and move forward with the knowledge that we can continue to iterate as we gather feedback from what is essentially, a form of internal research.
@annabeldunstone I'll read this over. I am know that there are still huge gaps in my knowledge about what exists currently since I was a very light user of the MR up to this point. If the feature already exists in the product, specifically reviewed/not approved, does your proposal change how that behaves at all? I guess I need to go back and figure out what the actual proposal is for beautifying our UI versus what already exists. Obviously the badge count and grayed out MRs is new. What else? cc @andyvolpe
@mnearents Thanks for taking a look and providing your feedback here. I'd agree that some of this feels like a large change and potentially something we'd want more feedback on long term as part of a larger review rounds effort - but I do think this helps move the ball forward.
From a concern perspective - is there anything you see in here that you think would negatively impact our ability to iterate and refine further or lock us in to a decision that we may not be easily able to change in the future?
If yes to that, I think we should be more cautious, but if there's nothing here that fits that criteria this could be a great way to get more feedback about what other improvements we need as part of review rounds. WDYT?
@phikai I think the badge being inconsistent with the number of items in the needs-review list (because the grayed out "done" reviews are mixed into the same list as needs review) is a concern. I don't know that it backs us into a corner iteration-wise. But I'm not saying it doesn't either. None of the notification/alert features in GitLab behave that way (to my knowledge), so it would just be a matter of time before we have to change it again. Finished Todos go into a "done" tab. Assigned issues that are closed go into a "closed" tab. Likely the better solution is a "Reviewed" tab or filter.
Todos do have a grayed-out state, but that's a temporary state when you manually hit "done". I think that's there just so the todo doesn't disappear the second you click done, which would be confusing. But on the next page refresh, it's now moved to the "Done" tab. Even if we implemented a "Reviewed" tab for MRs, I'm not sure I see us giving users the ability to approve from the needs-review list (not sure how that would make sense, why would you finish a review but not approve it right away from the MR page?), so I'm not sure we'd ever use that grayed out state. So we may be putting effort into a temporary feature.
@mnearents@phikai We could add a tab instead, but that feels like a much bigger change
So in the above screenshot, the Merge requests button in the side nav say 5 because you have 5 review requests, and it doesn't include the 1 that you have reviewed but is still open.
Also note- this doesn't take into account the MRs you have assigned to you (in the example above there are 0 assigned to me). Those will continue to be a part of the counter, and will need to be addressed in the larger review rounds effort.
@iamphill What are your thoughts on how much effort a tab would be vs the Reviewed state I originally proposed?
@annabeldunstone@iamphill@mnearents - I don't think we should do tabs because tabs currently correspond to state. The only states an MR can be in are Open, Merged, Closed. Until we introduce other states, we shouldn't confuse tabs with filters.
So back to the original proposal then. @mnearents Is your main hesitation with the fade out styling? If so, what if we just use the badge stylingwithout the faded content?
@phikai that makes sense on the state, but in that regard are we forcing our user's mental models to conform to our backend? Or are states what our users actually expect to see when looking for MRs they need to review? That would be my question. If we have this problem of having to remove ourselves as reviewers to keep our list clean (not ideal) I would question whether states are the best way to display MRs. It could be, especially for non-code review jobs to be done, I'm just questioning it is all.
@annabeldunstone I don't think my concern is that the design is wrong or bad. I guess it's that I just became the DRI for review rounds, doing research to approach this problem, while at the same time things are changing because of the beautify our UI effort and I don't know how to approach review rounds while so much is in motion. I do have concerns about the designs but they could be way off base or totally valid. I just haven't had time to do the research and think through the problems. I'd rather take the approach of doing problem discovery and designing specific solutions to solve specific problems, than making a change and seeing what happens. I'm not trying to have long toes, I just want to make sure we're doing the right thing as far as we have research insights to justify the changes and all this is happening while I'm trying to ramp up on current functionality and problems. Maybe you could help me understand where this solution came from and whether it has gone through solution validation, which may be easier/faster than building it. But I am open to being wrong and figuring out the best way forward.
@andyvolpe@pedroms Do you think this is the best way forward? And @phikai do you really feel this is proposal is a step forward based on all your prior knowledge and experience? Totally fine if so. I'm also willing to think through this if the work is handed back to review rounds.
forcing our user's mental models to conform to our backend
@mnearents I think those are all good questions, and things we should answer as part of a longer review rounds research and design effort. Largely, no... but it's also what we have. It's something Gabe has been thinking about in a concept called Status. What I think today is that our States are probably correct, but we don't have a way to communicate Status.
do you really feel this is proposal is a step forward based on all your prior knowledge and experience?
Yes - I don't think it negatively impacts anything we'd potentially want to do long term and is a step forward from what we have today. This strikes a good balance between iteratively shipping something and allowing us to continue to learn, research and refine.
I've been thinking a bit about how to improve commit-by-commit workflows (and maybe version-by-version too? Is that a thing? )
Maybe we could move the commit and version switchers into the file tree. And move the toggle file button into the tree as well, so that you can close the file tree from mid-way down the page (you still can't reopen it- separate issue).
This way, you can review a single commit, and then switch immediately to the next one without scrolling back to the top.
Commit by commit
Current
Proposal
Top of page
Scrolled down
Version switcher
Current
Proposal
Top of page
Scrolled down
Design notes
To get things to fit in the smallish, resizable file tree I removed some words. For example
Show latest version ==> Show latest on the commit view
latest version ==> latest on version switcher
I think we should still keep the full text in the dropdown itself. So it could still look like this:
The commit one is easy to do, but the version is a bit trickier The main you have in the screenshot can be another branch name if the user targets a different branch, so the dropdown text could be larger than the the area we have available.
I was wondering about that I guess we could truncate it but it would look a little silly depending on how narrow the user makes the file tree width. Maybe this isn't worth doing yet
There's some other work on this in #330579 - which also reminds me of #330579 (comment 1342845167)... I REALLY like this design... just wonder how we fit it in with the current "versions" concept.
@iamphill It looks like you are doing some work on this issue, but I'm not really sure what is in scope and what is not. Have you looked at all at this: #387070 (comment 1329997197) ?