-
Notifications
You must be signed in to change notification settings - Fork 867
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
feat: Rollouts UI List view refresh #3118
feat: Rollouts UI List view refresh #3118
Conversation
Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
…lark/argo-rollouts into phclark-feat/home-ui-refresh
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3118 +/- ##
==========================================
- Coverage 81.85% 81.84% -0.02%
==========================================
Files 134 134
Lines 20556 20558 +2
==========================================
- Hits 16826 16825 -1
- Misses 2866 2869 +3
Partials 864 864 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Philip Clark <[email protected]>
Is it possible for these e2e tests to be flaky? My previous commit passed all tests, but last commit failed the 1.26 e2e test even though the change was completed unrelated (only a minor UI code change). Can we rerun the test (I don't believe I have permissions) |
Yea it looks like something failed at githubs level i restarted it |
Signed-off-by: Philip Clark <[email protected]>
…lark/argo-rollouts into phclark-feat/home-ui-refresh
@phclark Would you be able to provide a screenshot/video of the changes you propose? |
Signed-off-by: Philip Clark <[email protected]>
Hi @ashutosh16 ! I've got a video demo for you, showing off the new functionality. We've got this running internally in our organization with a hundred or so engineers using it daily, so I've got a fair amount of confidence things work as intended but I do also encourage you to test it yourself to see if the functionality makes sense. |
Would you able to confirm if these are intended change? look very promising.. |
favoritesMatches = true; | ||
} | ||
if (filters.showRequiresAttention && (r.status === 'Degraded' || (r.status === 'Paused' && r.message !== 'CanaryPauseStep'))) { | ||
requiresAttentionMatches = true; |
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.
sorter: (a: any, b: any) => a.strategy.localeCompare(b.strategy), | ||
render: (strategy: string) => { | ||
return ( | ||
<InfoItemRow label={''} items={{content: strategy, icon: strategy === 'BlueGreen' ? 'fa-palette' : 'fa-dove', kind: strategy.toLowerCase() as InfoItemKind}} /> |
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.
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 wasn't able to get this to work with center alignment, but I think it looks really clean when aligned left
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.
Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
return false; | ||
}; | ||
|
||
export const RolloutWidget = (props: { |
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.
Since there is already a component with same name. Would it be better to rename it differently.
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.
@ashutosh16 I've renamed my new widget to RolloutGridWidget
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'll review the PR today.
Signed-off-by: Philip Clark <[email protected]>
Signed-off-by: Philip Clark <[email protected]>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
LGTM
@zachaller I have verified the UI and the changes look good to me. Do you want to merge the PR? |
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 for the contributions!
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.