-
-
Notifications
You must be signed in to change notification settings - Fork 651
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
Ethereum Alarm Clock Integration #1343
Ethereum Alarm Clock Integration #1343
Conversation
5eb33da
to
42b6b91
Compare
UI Improvements (Ideas - To be discussed further)
|
aa4efeb
to
0b600d0
Compare
* Scheduling: Basic date and time widget * Linting fixes * Moved the datetime field to new tab * Fixed push errors * Added missing specs * Undid unintentional UI change * Fixed some failing tests * Ignore datetime parameter when checking if a transaction is full * Added a date selector widget and renamed ScheduleTimestamp to ScheduleDate * Marked componentDidMount * Initialized Pikaday * Revert "Initialized Pikaday" This reverts commit 4e5bf5b. * Revert "Marked componentDidMount" This reverts commit 85d5219. * Revert "Added a date selector widget and renamed ScheduleTimestamp to ScheduleDate" This reverts commit aaad0ac. * Converted the date picker into a datetime picker * Added decent styling to the datetimepicker * Added validation to the datetime picker * Fixed prepush errors for scheduling timestamp * Adjusted validation logic scheduling timestamp
* [FEATURE] Scheduling: Timezone selector * Removed zombie files
* Implemented time/block switcher fuctionality
* Reordered, renamed and centered scheduling elements * Added the toggle button styling * Class -> ClassName
value: gasLimit | ||
}; | ||
|
||
const scheduling: SetSchedulingToggleAction['payload'] = yield select(getSchedulingToggle); |
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 could be in its own selector isSchedulingEnabled
common/selectors/schedule/helpers.ts
Outdated
|
||
const calculateWindowStart = ( | ||
scheduleType: string | null, | ||
scheduleTimestamp: any, |
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.
any
type here
const depositValid = isValidScheduleDeposit(state); | ||
const timeBountyValid = isValidCurrentTimeBounty(state); | ||
|
||
return ( |
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 would be nice if large conditionals like these were commented in groups for maintainability
…ypto into feature/eac-integration
Thanks a lot for the merge - we're getting really close here! There are a few style issues that could be improved before this gets merged into develop:
|
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.
Sorry for the delay on reviewing, I'm going to dismiss be previous request changes. Found a few other things in here, but other than the NewTabLink, I think everything looks good here. Ran a test on Kovan and it worked great.
interface InputTimeBountyIntentAction { | ||
type: TypeKeys.TIME_BOUNTY_INPUT_INTENT; | ||
payload: string; | ||
} |
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.
Leftover unused actions
@@ -30,6 +30,8 @@ export enum TypeKeys { | |||
GAS_LIMIT_INPUT = 'GAS_LIMIT_INPUT', | |||
GAS_PRICE_INPUT = 'GAS_PRICE_INPUT', | |||
GAS_PRICE_INPUT_INTENT = 'GAS_PRICE_INPUT_INTENT', | |||
TIME_BOUNTY_INPUT = 'TIME_BOUNTY_INPUT', | |||
TIME_BOUNTY_INPUT_INTENT = 'TIME_BOUNTY_INPUT_INTENT', |
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.
Leftover unused actions
let scheduleDetailsBtn: React.ReactElement<string> | undefined; | ||
if (scheduling) { | ||
scheduleDetailsBtn = ( | ||
<a href={getTXDetailsCheckURL(txHash)} className="btn btn-xs"> |
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 should use the NewTabLink
component.
|
||
&-field-title { | ||
position: relative; | ||
white-space: nowrap; |
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.
Fixed in: 5624221
if (windowStart) { | ||
this.props.setCurrentWindowStart(windowStart); | ||
} else { | ||
this.props.setCurrentWindowStart(latestBlock); |
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're going to need to kick off a fetch for latest block somewhere. Right now I think it only gets fetched when you change nodes. Looks like the default value is '???'
.
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.
Alternatively we could just start with an empty field.
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've removed the default value and we'll start with an empty field.
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.
All of my comments have been addressed, this is. a 👍 from me. Looking forward to mainnet!
|
||
return ( | ||
<div> | ||
{timeBounty && timeBounty.value && timeBounty.value.toString()} + {gasPriceWei} *{' '} |
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.
No need for the .toString methods here, they'll get converted to a string regardless
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.
Seems they don't get converted implicitly. Removing the .toString methods results in an error:
Objects are not valid as a React child (found: object with keys {negative, words, length, red}). If you meant to render a collection of children, use an array instead```
@@ -69,17 +86,50 @@ class FeeSummary extends React.Component<Props> { | |||
); | |||
|
|||
return ( | |||
<div className="FeeSummary"> | |||
<div className={`FeeSummary ${scheduling && 'SchedulingFeeSummary'}`}> |
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 boolean will add false
to the classname if scheduling
is false
Description
Integrate with Ethereum Alarm Clock to provide a way to schedule transaction.
Changes
Steps to Test
Screenshots