- Notifications
You must be signed in to change notification settings - Fork928
chore: refactor schedule banner#4274
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| return <FullScreenLoader /> | ||
| } else if (!template) { | ||
| return <FullScreenLoader /> | ||
| } else if (workspace && workspaceState.matches("ready")) { |
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.
Is not something we could use the Choose one component?
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.
Ooh good catch!
| // }, | ||
| // clearScheduleBannerRef: assign({ | ||
| // scheduleBannerRef: (_) => 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.
Should we remove these?
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.
yes thank you! Makes me realize - I tried both ways, spawning and invoking, and I wrote that I spawned but I really invoked! Haha, you can see why, invoking requires less code.
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. Nice refactoring!
Uh oh!
There was an error while loading.Please reload this page.
deadline, which comes fromworkspace. But the workspace page machine is huge. So instead of inlining the schedule banner machine inside the workspace page machine, Iinvokeit. This requires breaking the workspace page component into two components, because you can use a hook (to access the child machine) inside a conditional (which ensures the child machine has been invoked).I notice there's a pre-existing bug where we show a success message even if you try to increase (or decrease) beyond the max (or min) and so no change takes effect; something to fix later.