-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add new pipelines UI with promotion information #3510
Conversation
fc60712
to
5bbed22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, no major things.
? `${target?.clusterRef?.namespace || 'default'}/${ | ||
target?.clusterRef?.name | ||
}` | ||
: configResponse?.data?.managementClusterName || 'undefined'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want the word undefined
to appear for the cluster name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is some of the old code, I'm very confused by the useGetListConfig
stuff in general but I assumed that this literally meant that there's no management cluster. Same as your previous comment though - if it IS somehow a false
value we probs shouldn't display any links or anything. @yiannistri do you think showing something like unavailable
or cluster not found
would be appropriate here?
: 'alertOriginal' | ||
} | ||
> | ||
LAST APPLIED VERSION: {'V' + workload.lastAppliedRevision || '-'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want a little v
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design has a big V but I'm certainly not against a little v since that's how versions are typically written @mmoulian
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's apply lower-case v cc @mmoulian
<Text bold>Branch:</Text> | ||
<Text> {branch}</Text> | ||
<Text bold>URL:</Text> | ||
<Link to={url}>{url.split('.com/')[1]}</Link> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am worried this will only work for a narrow subset of git providers. Assuming we are trying to show the word github
or gitlab
, do we have any code that detects a git provider from a URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually wanted to split the provider off of the url but we certainly don't have to. The main thing was that there is not a lot of space for the url, and I was hoping to get as close to the repo name as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main thing was that there is not a lot of space for the url
Let's raise this with our designers and let them know the URL field needs to support like 50 characters or something in that range. We can get creative with ellipses if it overflows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @mmoulian please see previous comment ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main thing was that there is not a lot of space for the url
Let's raise this with our designers and let them know the URL field needs to support like 50 characters or something in that range. We can get creative with ellipses if it overflows.
We can change the layout and put the URL on another line, anyway my column is 268 px (@joshri I don't know if you use the same measurements) and it only supports 24 CH, therefore we need two lines for 50 CH.
height: ${cardHeight}; | ||
color: black; | ||
background: ${props => | ||
//use remainder operator to alternate between items in envColors array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confused by this. Are we trying to assign colors based on the environment, hoping that someone has selected 3 or less?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design has three colors for the environments, but it's possible to have more than three environments in a pipeline. I decided to just alternate between those three colors in the order displayed in the design, if that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design needs to take the data model into account. The color model falls apart on more than 3 stages of the pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @mmoulian ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design needs to take the data model into account. The color model falls apart on more than 3 stages of the pipeline.
From my point of view, colors have the role of creating distinction between environments and helping the user to identify them more easily. I think if users can only see three columns at a time, it's not a problem that we only have three colors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmoulian Consulted with Yiannis and Michael on this and we agree that, when it comes to adding color to environments, we should use color only to show the environment where the current change is being deployed and not color in any of the other environment columns.
With accessibility on my mind now, I'm thinking that the light yellow would be best so we can produce a higher contrast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With accessibility on my mind now, I'm thinking that the light yellow would be best so we can produce a higher contrast.
@LappleApple I will check the contrast of each color and tell you how it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mmoulian. I've suggested that we standardize our scheme https://weaveworks.slack.com/archives/C2YMKB137/p1697634836928619?thread_ts=1697632447.003429&cid=C2YMKB137
@joshri Thanks for your great work. Only one comment, I know that we style buttons is 2 pick round corner but I would like to these buttons would be different (they are inside a card). Can we do the corner radius 16px or totally rounded? Also change the color according to the design |
@mmoulian do you want the look to change when the button is disabled? Or is the plan to have the icon do the heavy lifting there |
5bbed22
to
f87fe2b
Compare
I understood that when we choose Manual approval Pipeline the button is always enabled (for that reason I created only two states enabled and hoover). When we choose Automated Pipeline we don't have button, so we don't need disabled button. (@LappleApple does it correct?) |
@LappleApple I'm thinking now. What about the environments? Can the user approve all at the same time? Or do you need to approve one by one? Does it mean that all environments are active at the same time? |
@mmoulian You approve envs one by one. This is a pipeline so as we go from left to right we promote env by env. |
@LappleApple @mmoulian The approve button will only appear in the case of manual approvals, but my understanding was that there is a case where the env would not be ready for promotion, and the button should be disabled - in the current structure of environments, it happens if the |
@joshri; Thanks for bring it up. @LappleApple I think the button must be enabled only when the env is ready to promotion. For example in this case the Stg env has the button disabled |
f87fe2b
to
58e4036
Compare
c106bb4
to
d2d27d7
Compare
Closes #3312
Closes #3313
Closes #3324
Figma link: https://www.figma.com/file/IVHnM9iyeFWpd11evtY8ux/Weave-GitOps?node-id=19656%3A13129&mode=dev
Faked Manual Pull Request Approval:
Example Video:
PipeUi.mov
How to test locally:
WEGO_EE_PROXY_HOST=https://pipelines.wge.dev.weave.works yarn start
with demo01 cluster loginThis PR gets the Pipelines UI about as close to the new designs as I can get it with pending backend changes on the way.
Expected remaining work: