Add badge to workflow history group to display time remaining #1036
Conversation
adhityamamallan
force-pushed
the
time-remaining-badge
branch
2 times, most recently
from
e10707b to
d4b5ae2
Compare
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
adhityamamallan
force-pushed
the
time-remaining-badge
branch
from
d4b5ae2 to
55f270f
Compare
| import WorkflowHistoryRemainingDurationBadge from '../workflow-history-remaining-duration-badge'; | ||
| import type { Props } from '../workflow-history-remaining-duration-badge.types'; | ||
|
|
||
| jest.mock('../helpers/get-formatted-remaining-duration', () => jest.fn()); |
There was a problem hiding this comment.
nit: no need for the passing the second argument function, mock does this by default
| const formatDuration = ( | ||
| duration: Duration | null, | ||
| { separator = ', ' }: { separator?: string } = {} | ||
| { separator = ', ' }: { separator?: string } = {}, |
There was a problem hiding this comment.
minUnit can go with separator, The second argument is meant for all optional configurations
There was a problem hiding this comment.
Ahh, got it, that's good
| const remainingNanosAsMillis = nanosAsMillis % 1; | ||
| const milliseconds = secondsAsMillis + intMillis; | ||
| const units = ['y', 'M', 'd', 'h', 'm', 's', 'ms'] as const; | ||
| const allUnits: Array<dayjs.ManipulateType> = [ |
There was a problem hiding this comment.
Why not use the same type on props ?
There was a problem hiding this comment.
['y', 'M', 'd', 'h', 'm', 's', 'ms'] is a subset of dayjs.ManipulateType, which also includes values like 'seconds', 'milliseconds' and such.
https://github.com/iamkun/dayjs/blob/80401e3ff91cc6c5310e6603a4d7a5fa92ca90ec/types/index.d.ts#L32
There was a problem hiding this comment.
If we can add the type to the types and reuse it it would be the correct way.
| expect(setIntervalSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('clears existing interval when shouldHide becomes true', () => { |
There was a problem hiding this comment.
Nit: test case title is not clear as interval is used differently in above tests, can be changed to doesn't render duration when shouldHide becomes true
|
Btw, the description says that group has field |
The new value doesn't have wait durations, it instead has the absolute expected end time. I am okay with either, but unfortunately the old PR had the same mistake. Do you want me to change it back? |
|
Yes, i noticed it in the prev PR. But reading it in usages made it more clear that it can be understood as expected event end time. |
|
Sounds good to me, would it be okay if I made this change in another PR? |
2 participants
Summary
waitTimerInfois set for the event groupformatDurationutil to accept a minimum unitTest plan
Added/updated unit tests + ran locally.