With 9.5 we have new visualization for MR widget, specifically for Approval and when MR is WIP:
When I first saw this I found it surprising. The red X icon was previously used only to show that a pipeline failed.
Now that it is used in multiple places, I keep getting confused and keep going to check why the pipeline failed.
I don't know what a better solution would be, but I want to raise this concern as we might want to think about a better icon for these actions.
Thanks for making this issue @winh, I agree with you on the 'X' icon.
I don't feel that it is a failure that someone did not approve, it should be informative notification.
I agree completely here @marin. @dimitrieh, can you give us some insight on the choice of icon here? I tried to find the original issue for context but can't seem to locate it.
You can't start a MR in a state of failure. That would be a huge bummer. There is a difference between part of the flow of an MR and an actual failure. Red should be a destructive color. This is not a destructive situation. It is a neutral state. If you want it like a check list that's a different thing. And please don't use the build icons. I've been using GitLab for a long time and you can't suddenly switch those icons from being all about builds to being used for other things. How about this. Instead of using established icons.
And please don't use the build icons. I've been using GitLab for a long time and you can't suddenly switch those icons from being all about builds to being used for other things. How about this. Instead of using established icons.
I agree with @jschatz1 on this one. I think that we need to be careful when appropriating symbols from established areas. There may be instances where it will align the UX and be beneficial, I don't think this is one of those instances.
I like the solution Jacob proposed @dimitrieh, I know you had ideas at one point that were similar. Let's investigate how this may work better for this situation.
After discussion at https://gitlab.slack.com/archives/C03MSG8B7/p1502816336000401 it becomes clear that reusing the more general CI icons to indicate something wrong, or success is not the way to go... as these icons are too ingrained with CI pipelines specifically.
Good point @arcresu ! This is the part that makes it difficult :). Basically, the situation is the following:
We need a standardised way to indicate "success", "fail", "warning". These need to be different from the pipeline icons, but flexible enough to be used in multiple places.
Color alone is not a complete option, as this is not inclusive design, thus an icon is needed.
I think the circle around the icon indicates that it is a job or pipeline (something ci related). Therefore we may be able to change this slightly..
Because there are multiple ways to look at this problem. I have been breaking my head over this, this afternoon trying to fix it along the terms I set in https://gitlab.com/gitlab-org/gitlab-ce/issues/36487#note_37686486, however, the more I worked on it, the more I am convinced that it may not be the right way... And the correct way was the one I intended for with my original change, using the icons for more purposes.
So it all comes down to assumptions. Which assumption is right? Which facts do we have?
We should be able to see the pipeline status from the MR list (as this feature should not be lost)
We are already using the same "succeed/passes" icon in other places in the application:
Resolving discussions
We are using it for both jobs and pipelines
We are using it for deploys
we are using it for code climate statuses
We may use it in the future to some extent to indicate if a MR is merged yes or no.
So my intention with the original change that introduced this issue, is to decouple a few of these circular icons (success, warning, failure) to specifically target pipelines only, and rather have them be used for more places to indicate status.
This change was initiated as we are already moving beyond just pipelines! We already have them indicate deploys and code climate status in the MR widget. This move was to continue that trend in order to have a glanceable view of the mr widget. Making it more efficient to indicate if someone needs to be done.
Looking at GH, they have this same concept going on:
So now we come to the assumptions part. We have the feature to be able to see the pipeline status on the mr list. However is this still enough? What about codeclimate? What about mergability? What about deploys?
They all should be able to be represented, and limiting our selves to just using this "succeed/passes" icon for pipelines is not going to help.
Let's take a look at how GH is solving a sort of similar problem:
We can see it is not referencing only ci pipelines, but rather checks the health of the complete MR. (quick fact: gh doesn't indicate merge conflicts/mergeability with this).
Solution
1/2 solution (what needs to happen in the short term)
For Approvals and Merge status, change the icons usage to use the warning icon rather than the failed icon.
2/2 solution (what needs to happen in the long term)
I would like to expand our MR list mr status indicator (which is now either used to indicate pipeline or merge conflicts). The way I see this working, is with a rich tooltip:
zoomed in:
I am going to unlabel this regression as it can be considered an iteration, but is maybe rather a good example of UX debt. I would still like to see this implemented sooner rather than later.
Wouldn't the proposed design make it much slower to parse for someone with red-green colour-blindness?
This is an excellent point @arcresu. It is such an easy thing to overlook but a very important consideration.
For Approvals and Merge status, change the icons usage to use the warning icon rather than the failed icon.
This is an improvement. The failed icon was confusing, and I think, what really sparked questioning whether re-using these icons was a good idea in the first place.
Because there are multiple ways to look at this problem. I have been breaking my head over this, this afternoon trying to fix it along the terms I set in https://gitlab.com/gitlab-org/gitlab-ce/issues/36487#note_37686486, however, the more I worked on it, the more I am convinced that it may not be the right way... And the correct way was the one I intended for with my original change, using the icons for more purposes.
I appreciate all the thought and work that went into explaining your reasonings here. I was strongly in @jschatz1 camp to start with but I do see value in your argument. @jschatz1, after reading @dimitrieh breakdown, how do you feel about this?
We need a standardised way to indicate "success", "fail", "warning". These need to be different from the pipeline icons, but flexible enough to be used in multiple places.
@dimitrieh This is a significant issue. It's mentioned elsewhere that we don't want to (ab)use pipeline icons for non-pipeline things, but I think that's a false reaction. The problem is that we used the wrong icon. Pending approvals is not a failure.
So then it's suggested to use warning, and that is certainly better, and may indeed be good enough. But it's worth considering that pending approval is also not a warning. It's natural state in a healthy flow. No reason for concern; just something that's not done yet. Like an open checkbox. Insert your own graphic design choices here, but the point is it's not a state that is represented by our current icons.
Now, there's another thing we're glossing over. We keep talking about the ability to merge as being the most important thing about this page. And given that it's a merge request, that logic sounds reasonable. But let me throw out some counter points. If I'm a developer trying to debug a test failure, my sole concern is the pipeline status. A warning about not having approval is premature and distracting.
Going further, once a MR is ready for peer review, the reviewer doesn't need to see the lack of review as a failure or warning. They're looking for some other status entirely. Perhaps just pipeline status, but also code quality analysis, etc.
When someone is finally ready to merge, yeah, that's where you want to quickly assess the merge-ability of the MR.
But here's the thing. Each of those people is probably focusing on a different part of the interface.
In our effort to connect the cause to the effect, we forgot to actually draw the connection. We put an X near the approval, but it's in isolation. There's no connection to the merge button other than proximity.
If the merge button has text associated with it that says it can't be merged because it's not approved, the connection is obvious. Earlier mockups included lines literally connecting cause and effect. If you separate these things on different lines, I now need to parse/deduce the relationship.
I'm not sure I'm being clear. It makes a lot more sense in my head. :)
Trying another way, we've confused state with cause/effect relationships. The state is that the MR is not approved yet. The effect is that you can't merge. (And X is not a valid indicator for either piece of information.)
But tying this with the above, if you assume that the only reason I'm on the page is to merge, well then a bunch of icons indicating whether I can merge or not might seem reasonable. But that's not the only job this page has. When I come to the page to do a different job (e.g. check if my pipeline passed), that list of states makes less sense.
Side note, but when something isn't approved, why are we encouraging people to work around the approval by merging manually via CLI? (I know that's not really why we mention it, but it feels totally wrong.) Moved to https://gitlab.com/gitlab-org/gitlab-ce/issues/36634.
It's mentioned elsewhere that we don't want to (ab)use pipeline icons for non-pipeline things,
@markpundsack this is exactly the thing that is not a fact but an assumption. Of course abuse would be wrong, but isolating icons to be used for one thing only, while they are pretty general in meaning is wrong in my book (therefore my big explanation in https://gitlab.com/gitlab-org/gitlab-ce/issues/36487#note_37727714). Although I see you know what i mean down the line in your comment ;) )
What I mean to say, the icon alone should not indicate "pipeline", it should be a combination of icon and context that should create this connection.
Now, there's another thing we're glossing over. We keep talking about the ability to merge as being the most important thing about this page. And given that it's a merge request, that logic sounds reasonable. But let me throw out some counter points. If I'm a developer trying to debug a test failure, my sole concern is the pipeline status. A warning about not having approval is premature and distracting.
This was not something I forgot, however the counterpoint can also be made: what if we would focus on review alone? Then merging would be forgotten. The issue with the mr view is that it complements so many different flows as you indicate. Making it so, that one doesn't scream for more attention over the others is key I think.
Making the Failure icon into a warning icon, is exactly such a measure to solve that partly. Still we could perhaps do slightly better even, because if we look at GH, they grey out that specific icon:
So it is maybe a good idea to do this, which would result in:
If the merge button has text associated with it that says it can't be merged because it's not approved, the connection is obvious. Earlier mockups included lines literally connecting cause and effect. If you separate these things on different lines, I now need to parse/deduce the relationship.
@markpundsack I think we are on the same line here, but not totally sure
Btw back in https://gitlab.com/gitlab-org/gitlab-ce/issues/25424#note_22751855 I talk about making this split somewhat more clear btw, I am not entirely sure its needed, though.. but it's fun to show a possible result. It does solve the problem with the weird header, that's only valuable information when you are inspecting the branch or merging it, plus it brings it closer to the merge button. Thoughts?
@markpundsack i think we all agree on changing the icon (which I will update the issue description with), however what do you think of the MR list view pop-out mr status widget:
@victorwu seems good to me, although I would like to see the product team give UX debt a higher priority than normal (as it can be considered a sort of regression)
Most of the time UX debt is a regression of a newly implemented feature.. thus we cannot say it went backwards in terms of functionality, however it doesn't meet the full mvp of said implemented feature.
The time between "new feature implementation", UX debt issue created, and fix should be as short as possible (similar to regression )
I'm not sure I'm being clear. It makes a lot more sense in my head. :)
This makes a lot of sense, I appreciate the write-up!
When I come to the page to do a different job (e.g. check if my pipeline passed), that list of states makes less sense.
This pinpoints my biggest issue with the MR widget in general. I am presented with this laundry list of items all asking for my attention. It is confusing and difficult to parse quickly.
Something jumped out at me when looking at the (somewhat blurry)image of the GH MR, there is a clear visual hierarchy there @dimitrieh. There is a distinct heading indicated by the larger icon. I am then brought down the list visually using smaller icons, indicating different stages that have or have not passed. I don't believe that changing the icon or color used will solve this problem without also creating a better overall hierarchy.
@dimitrieh My gut reaction to your design in https://gitlab.com/gitlab-org/gitlab-ce/issues/36487#note_37912135 is that it looks nice. But it also only shows one case, where there's a clear separation between successful items and a blocking condition. To properly evaluate, you'd have to look at all the permutations. Or more specifically, look at the various flows.
@dimitrieh The pop-out mr status widget might work, but I fear it's more of the same problem. It's reducing several different pieces of information into a single status, which may not actually be a status I care about at that time.
I don't believe that changing the icon or color used will solve this problem without also creating a better overall hierarchy.
As a thought experiment, I'm trying to reduce my questions into categories or flows.
Developer writing MR
Reviewer reviewing MR
Maintainer merging MR
Project manager assessing status
Product Manager evaluating MR results (in review app and/or in production)
I don't know if it's tracked anywhere, but I know @dimitrieh and I have talked about hiding information once an MR goes from one stage to another. Like after the MR has been merged, the deployment status is important, but the CI test status and approval status isn't and could be hidden. Is there an equivalent for the earlier stages? In some ideal world, we'd know why you opened the MR and show you only the information you care about. Maybe we detect the viewer as being the assignee and show different things. I don't know.
In the absence of that clarity, I think we need to aim for presenting all the information without favoring one flow to the point of making the other flows suck.
The pop-out mr status widget might work, but I fear it's more of the same problem. It's reducing several different pieces of information into a single status, which may not actually be a status I care about at that time.
True, but until we solve the problem of predicting what flow the user will be in/ what information he/she is after.. this will not be solved. This is the same problem that is holding back the UX Research issue on MR's @sarrahvesselov
There is a distinct heading indicated by the larger icon. I am then brought down the list visually using smaller icons, indicating different stages that have or have not passed. I don't believe that changing the icon or color used will solve this problem without also creating a better overall hierarchy.
@sarrahvesselov@markpundsack Although I really like the idea, this is again prioritising one or two flows over the others.
Developer writing MR
Reviewer reviewing MR
Maintainer merging MR
Project manager assessing status
Product Manager evaluating MR results (in review app and/or in production)
@markpundsack there is another one I think, not in this list:
Performance maintainer
How is this performing vs previous deployments
If more key performance indicators become available due to advanced monitorring, I could even foresee a product manager looking there for how popular a feature is vs another/ retention increase. etc
So do we think merge is above the others? Is approval above the others? CI is really important for most developers at the moment. etc etc
So the problem still stays what does the current user need? Until that is solved, nothing I come up with will be great.. it will always be a compromise.
@markpundsack@sarrahvesselov while we ponder more about that topic I would suggest going with the easy fix I propose with changing the icon to warning icons, which are grey (I updated the issue description with it).
So the problem still stays what does the current user need? How do we find out which tasks they need to do/which flow they are in?
How can we solve this? ux-research#21 (closed) also talks about the possibility to let the user configure the MR for them personally, for example to have "Description expansion" yes or no.
We don't want to push someone everytime they open an mr into choosing what flow they are in, but what potential idea's are there/can we think of.
I put this together to better explain the hierarchy I think we should be presenting. I broke it down into very basic shapes in the hopes of focusing more on the structure than specific contents.
Although I really like the idea, this is again prioritising one or two flows over the others.
I disagree here. Giving users information in a way that is easy to understand and parse will allow them to consume the information most relevant to them. We are not prioritizing a flow, we are allowing users to 'see' the information most important to their personal interests.
This is the same problem that is holding back the UX Research issue on MR's @sarrahvesselov
I am unaware that anything is holding back this research. The research is complete, we need to take the results and find ways to improve the MR based on those results.
True, but until we solve the problem of predicting what flow the user will be in/ what information he/she is after.. this will not be solved.
@dimitrieh Sure, so don't make things worse in the meantime. :) The current commit indicator works for at least one flow. Don't make work for zero flows while attempting to make it more balanced.
Giving users information in a way that is easy to understand and parse will allow them to consume the information most relevant to them. We are not prioritizing a flow, we are allowing users to 'see' the information most important to their personal interests.
@dimitrieh@sarrahvesselov This is key. If we can't guess which flow is important, make it easy for them to figure it out.
Rethinking the flows, and building on @dimitrieh's design, how about a hierarchy showing:
Overall MR status (Open - WIP, Open - In review, Open - Ready to merge, Merged, Merged - Shipped)
Status of Code (or Status of CI, or Status of Pipeline)
Status of Deployments
Status of Review (Approvals, unresolved discussions)
Merge button (+ reasons it can't be merged)
Basically, make it so anyone following one of those flows has only one place to look. Yes, the other statuses will be competing, but at least their reasons are clear, so you can quickly determine which to focus on and which to ignore. Memory will make this even better.
Some further comments:
In the above, I'm assuming that even if an MR can't be merged because of, say, the status of review, that the specific reason(s) are denoted at the merge button itself. This way if I'm assessing whether it can be merged, I look in one place. If I'm working on the review itself, I look in one (different place). Some of the information might be redundant, but it's information where you need it.
The status of deployments part is tricky. @dimitrieh and I have talked about breaking up pre-merge deployments from post-merge deployments because while they're both deployments, they're really for different purposes. So the status of a review app would be up near the status of code, or even in the status of review part (as it's needed for reviewing), and the staging/production deployments could be after the merge button. With that in place, the various statuses would be roughly chronological in most flows. e.g. you get the code passing CI before you ask for peer-review.
So a better breakdown is:
Overall MR status (Open - WIP, Open - In review, Open - Ready to merge, Merged, Merged - Shipped)
Status of Code (or Status of CI, or Status of Pipeline)
Status of Review (Approvals, unresolved discussions)
Merge button (+ reasons it can't be merged)
Status of Shipment (staging/production deploy, including metrics)
Which reminds me, I added more granularity in the overall MR status. Today, we only have open, merged, and closed. But it's really more granular than that when you consider WIP status, approvals, discussions, and deployments. Much like in Issue Board discussions, it kind of sucks to leave readers to deduce those various conditions. We could make it easier and provide an overall status. My suggestion is to make the minimal change by leaving primary status as open, merged, closed, but add sub-statuses.
Lastly, some of these statuses don't make sense with a uniform checkmark. I know earlier designs hinged on having each section have a checkmark if it's all OK, and X if not. But some of these aren't binary. And in the earlier designs, the checkmarks were all answering questions around a specific focus of whether it can be merged or not. Removing that non-goal, each section should be designed for the flow it supports. So status of code is usually pass/fail/warn, but status of review is more like done/not-done (or ready for review, review started, accepted, rejected). The merge button section does not need a status icon at all. It's either disabled/enabled, or already merged.
Basically, make it so anyone following one of those flows has only one place to look. Yes, the other statuses will be competing, but at least their reasons are clear, so you can quickly determine which to focus on and which to ignore. Memory will make this even better.
Yes! @markpundsack, this is exactly what I was getting at.
There are two things that have always quite bothered me about our MR. One is the lack of clear hierarchy which I have stated here. Another is our choice of button location. Placing disabled buttons before text that explains why it is disabled is not a good UX IMO. I added a wireframe here to illustrate what I envision may be a better layout.
@bikebilly : This is a ~Discussion issue I'm pretty sure, as long as the final design/scope is about re-arranging the existing elements in the merge request widget, and re-styling them. (Meaning the ~Discussion team should work on it.)
It's currently scheduled for 10.1. But let's see what the final design is and we may want to slice and dice it some more.
My proposed solution still works @arcresu@dimitrieh@sarrahvesselov. Color blindness is a problem when one can't identify the intent of the button because it is only defined by the color itself (like MMN). In my proposal the colors are an added for clarity. We should choose different colors as we should all around the site for red green color blindness. This is no different than our labels, and all our buttons, the colors are not directly needed to associate the meaning. The panel still says "1 approval needed". And "Merge conflicts".
I agree with @markpundsack that the colors and symbols were completely wrong. I said that above, that even a merge conflict is not a failure... Red should only be used for destructive non reversible things. Closing an issue is not destructive. Closing an MR is not destructive. All those things are reversible. Red should not have been used for any of the situations above. I was using it to repeat what was done. But the X further says "destructive" and also should not have been used.
The right handed stripe is just a label with it's color moved to the side.
@jschatz1 I do not fully agree here. What I am trying to do here is that without much or any reading (thus glancing) you should be able to discern the status of each item.
To make this use-case more clear: Say that all of the tests and rows on the mr-widget are green. If that is the case, mostly you wouldn't even care what they are.. as everything is fine. However if we rely on just color (red/green) this information is not very accessible, loosing that functionality.
A good example to show this off and see what color-blind people can experiencing in a simplified way is to make your example black and white:
Today I am going to give this another try and find a solution that does not try to do everything equally, but rather give a path for people to follow as suggested in the comments above.
Created https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14054 as a short term solution for this issue, it changes the severe failed status icons to the more soft warning version for mr warning messages that fit it (so that can go in 10.0).
I propose we branch this issue of in another issue for 10.1.. will update later
Today I am going to give this another try and find a solution that does not try to do everything equally, but rather give a path for people to follow as suggested in the comments above.
We should create a new issue to improve the hierarchy in MR. The discussion here has gotten long and difficult to follow. We can simply address the icons/color used here to reduce the confusion caused and work on iterating the rest.
@dimitrieh : For GitLab team members' work, we follow a strict scheduling process. Please read this: https://about.gitlab.com/handbook/engineering/workflow/#product-development-timeline very carefully. Engineering teams are now working very hard to complete work as originally scheduled. We should not disrupt the process especially toward the end. Non-engineers (in role) and community members should definitely be encouraged to contribute code. I'm not aware of a strict process to get those reviewed and merged. But it's definitely a safe idea to ping the engineering leads if you want help to get something merged not originally scheduled. (So @jschatz1 in this case I think makes a lot of sense.)
@dimitrieh grab any member of the team that is not a maintainer for FE review. Then can review first then pass to a maintainer. Check the team page to find the right candidate.
@dimitrieh: I updated the description above the be more generic, and scoped out https://gitlab.com/gitlab-org/gitlab-ee/issues/4133 specifically for an upcoming release to be worked on. That one is just for approvals for approvals only. We can keep this one open for continued discussion on the merge request if you think that's a good idea.