-
Notifications
You must be signed in to change notification settings - Fork 2
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: refactor public component wrappers for auto-answering into preview Node
#4108
Conversation
@@ -96,13 +104,16 @@ const Node: React.FC<Props> = (props) => { | |||
}); | |||
|
|||
switch (props.node.type) { | |||
case TYPES.AddressInput: | |||
return <AddressInputComponent {...getComponentProps<AddressInput>()} />; | |||
|
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 file is a bit extra noisy because I've alphabetized our component types within the switch
block - only Checklist
, Filter
, Question
types have functional changes!
|
||
import Question from "./Question"; | ||
|
||
export default ( |
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'm dropping this fixture
file as I believe it's unused and not a component pattern we follow elsewhere - any objections? Dropping this also allows to eliminate Public/package.json
Removed vultr server and associated DNS entries |
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.
Great! Thanks for getting back to this - exactly as expected.
Two very minor comments, no showstoppers!
editor.planx.uk/src/@planx/components/Checklist/Public/Public.tsx
Outdated
Show resolved
Hide resolved
const autoAnswerableFlag = useStore((state) => state.autoAnswerableFlag); | ||
|
||
let idThatCanBeAutoAnswered: string | undefined; | ||
if (props.id) idThatCanBeAutoAnswered = autoAnswerableFlag(props.id); |
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.
Nice! 💪
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.
Thanks for spotting this - this looks like just a weird artifact of somebody calling pnpm i
in this folder that we've missed at some point?
@@ -155,6 +173,13 @@ const Node: React.FC<Props> = (props) => { | |||
/> | |||
); | |||
|
|||
case TYPES.Filter: { | |||
const filterProps = getComponentProps<Filter>(); | |||
const autoAnswer = nodeId ? autoAnswerableFlag(nodeId) : 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.
nit: We could calculate this at the top of the file and add it to both getComponentProps()
and PublicProps<T>
.
Slightly cleaner / more consistent, and we could put a check for component type within autoAnswerableFlag()
if it's an expensive / slow function.
// These types are never seen by users, nor do they leave their own breadcrumbs entry | ||
case TYPES.Answer: |
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.
Always appreciate seeing some alphabetising!
Replaces outdated #3917
First step before moving onto support auto-answering Input component types (per logic followups outlined here) - this refactor sets up a more flexible, extensible pattern than per-component public "visibility wrappers" as initially implemented. Now, all auto-answerable node IDs are computed within the parent
Node
component and passed down to relevant component types only.Updates the following
@planx/components
:All existing tests still passing (Public component tests, automation logic unit tests, e2e).