-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
chore(deps): upgrade argo-ui
, including breaking changes
#19655
chore(deps): upgrade argo-ui
, including breaking changes
#19655
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
- upgrade to https://github.com/argoproj/argo-ui/tree/a7b01f9f009aa9161da21f450b5aab2f0732eb74 - `moment` is no longer used internally by `argo-ui`, so change `Ticker` usage to account for this - CD should also remove the deprecated `moment` library for the same reasons as downstream and Workflows Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
370a5ac
to
8f44ca8
Compare
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Ah type-checking failed as I appear to have forgotten to make
I'll push a workaround here (setting the other to |
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.
Huh since it already finished, that should've actually short-circuited the changed code here 🤔
Yea that would be helpful! |
Oh, this is an edge case: it's a 0 second duration apparently. I suppose that's a bug in my refactor; the previous logic did handle 0 on this line, although not sure if it intentionally handled that 😅 I'll throw in another fix in |
No wait, the new logic (or well, newly switched to logic, the logic itself is still old) handles the |
Oh, I think this will be fixed by argoproj/argo-ui#577 as well; the
And JS doesn't error when doing operations on The workaround here would be to use |
bc in JS `0 || null == null`, so the edge case of a `0` duration caused the component to render just an empty string Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
argo-ui
, including one breaking changeargo-ui
, including breaking changes
Now that argoproj/argo-ui#577 has been merged, I've updated the code here as well |
/bns:start |
/bns:deploy |
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.
Local test checks out, thanks!
…19655) * chore(deps): upgrade `argo-ui`, including one breaking change - upgrade to https://github.com/argoproj/argo-ui/tree/a7b01f9f009aa9161da21f450b5aab2f0732eb74 - `moment` is no longer used internally by `argo-ui`, so change `Ticker` usage to account for this - CD should also remove the deprecated `moment` library for the same reasons as downstream and Workflows Signed-off-by: Anton Gilgur <agilgur5@gmail.com> * type-check and format fixes Signed-off-by: Anton Gilgur <agilgur5@gmail.com> * set `durationMs={0}` instead of `null` bc in JS `0 || null == null`, so the edge case of a `0` duration caused the component to render just an empty string Signed-off-by: Anton Gilgur <agilgur5@gmail.com> * update argo-ui once more for deprecated durationMs removal Signed-off-by: Anton Gilgur <agilgur5@gmail.com> --------- Signed-off-by: Anton Gilgur <agilgur5@gmail.com> Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>
Motivation
As requested in argoproj/argo-ui#535 (review)
Upgrade to the latest version of
argo-ui
, resolve breakage, and fix a bug in CD via the upgradeModifications
upgrade to https://github.com/argoproj/argo-ui/tree/a7b01f9f009aa9161da21f450b5aab2f0732eb74
Per refactor(deps)!: remove
moment
dep and usage argo-ui#535,moment
is no longer used internally byargo-ui
, so changeTicker
usage to account for thismoment
library for as it is deprecated. see also the downstream PR and Workflows PRs (refactor(ui): replacemoment-timezone
with nativeIntl
argo-workflows#12097, refactor(deps): removemoment
dep and usage argo-workflows#12611)Per fix!: remove deprecated
durationMs
prop argo-ui#577, theDuration
component had an erroneously nameddurationMs
prop, which has now been changed todurationS
Fixes #16745 via argoproj/argo-ui#546, which is included in this upgrade
Checklist: