-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Feedback/#11253 #11304
base: master
Are you sure you want to change the base?
Feedback/#11253 #11304
Conversation
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.
Overall, it looks great to me 🚀
Just have a couple of questions and suggestions.
@@ -267,7 +267,7 @@ export const getSaleParameters = ( | |||
coretime: 0, | |||
relay: currentRegionInfo.end.blocks.relay + get.blocks.relay(config.regionLength) | |||
}, | |||
date: estimateTime(currentRegionInfo.end.ts + config.regionLength, get.blocks.relay(lastCommittedTimeslice), constants.relay), | |||
date: formatDate(new Date(estimateTime(currentRegionInfo.end.ts + config.regionLength, get.blocks.relay(lastCommittedTimeslice), constants.relay) ?? '')), |
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.
It might end up giving Invalid Date
or some incorrect info if
const estimated = estimateTime(currentRegionInfo.end.ts + config.regionLength, get.blocks.relay(lastCommittedTimeslice), constants.relay);
if estimated
returns null
and the expression will eventually evaluate to formatDate(new Date(''))
.
However, If it is always guaranteed that estimatedTime
will not return null
, then we should update types accordingly.
@@ -164,11 +164,11 @@ const getPhaseConfiguration = ( | |||
const fixedPriceLengthTs = regionLength - interludeLengthTs - leadInLengthTs; | |||
const fixedPriceEndTs = priceDiscoveryEndTs + fixedPriceLengthTs; | |||
const get = createGet(constants); | |||
const getDate = (ts: number) => estimateTime(ts, get.blocks.relay(lastCommittedTimeslice) ?? 0, constants.relay); | |||
const getDate = (ts: number) => formatDate(new Date(estimateTime(ts, get.blocks.relay(lastCommittedTimeslice) ?? 0, constants.relay) ?? '')); |
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.
Same as #11304 (comment)
const chainRegionEnd = (chainRecord.renewalStatus === ChainRenewalStatus.Renewed ? regionEnd : regionBegin); | ||
const targetTimeslice = lease?.until || chainRegionEnd; | ||
const showEstimates = !!targetTimeslice && Object.values(CoreTimeTypes)[chainRecord.type] !== CoreTimeTypes.Reservation; | ||
const { coretimeInfo, get } = useCoretimeContext(); | ||
|
||
const estimatedTime = showEstimates && get && coretimeInfo && | ||
estimateTime(targetTimeslice, get.blocks.relay(lastCommittedTimeslice), coretimeInfo?.constants?.relay); | ||
formatDate(new Date(estimateTime(targetTimeslice, get.blocks.relay(lastCommittedTimeslice), coretimeInfo?.constants?.relay) ?? '')); |
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.
Same as #11304 (comment)
const currentRegionStart = useMemo(() => coretimeInfo ? coretimeInfo.salesInfo.regionEnd - coretimeInfo?.config.regionLength * 2 : 0, [coretimeInfo]); | ||
|
||
const saleStartDate = useMemo(() => { | ||
return get && coretimeInfo && formatDate(new Date(estimateTime(currentRegionStart, get.blocks.relay(coretimeInfo?.status?.lastTimeslice), coretimeInfo.constants.relay) ?? ''), 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.
Same as #11304 (comment)
}, [currentRegionStart, coretimeInfo, get]); | ||
|
||
const saleEndDate = useMemo(() => { | ||
return get && coretimeInfo && formatDate(new Date(estimateTime(currentRegionEnd, get.blocks.relay(coretimeInfo?.status?.lastTimeslice), coretimeInfo.constants.relay) ?? ''), 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.
Same as #11304 (comment)
@@ -46,154 +46,62 @@ const TableCol = ({ header, | |||
); | |||
|
|||
function WorkInfoRow ({ data }: { data: InfoRow }): React.ReactElement { | |||
if (!data.task) { | |||
if (!data.task || data.type === 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.
We can say it just !data.type
.
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.
data.type
is an enum, hence it starts from 0 :) Actually I just realized this check is due to a switch later down the file, that is removed now, so I will remove this too.
export enum CoreTimeTypes {
'Reservation',
'Lease',
'Bulk Coretime',
'On Demand'
}
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.
Alright. Thanks.
Addressing feedback from #11253 and a few points from #11262 (comment)
changes
to reproduce:
minor code changes