Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Doc(adr): move formFields from CMS into app #1339
base: main
Are you sure you want to change the base?
Doc(adr): move formFields from CMS into app #1339
Changes from all commits
2efe738
0144591
7a14e73
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'm still wondering if we could go one step further and not only represent pages this way but pages within whole subflows. Such as:
That would mean that we could validate whole sub-areas of the flow by checking only for
anwaltlicheVertretung
and gathering all fields in there.I don't know, probably a thought for the future as this is not that well thought-out, still wanted to leave it here.
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 not sure I follow - but I have to admit, nested schemas (and arrays) are still not prototyped using this approach.
Right now I think we could either encode them the way you showed (which matches our current
userInputSchemas
) or do it like in the CMS (eganwaltEntscheidung.hasAnwalt: z.enum(["yes", "no"]) }
).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 think that's essentially the same thing? With the first option just giving us more variety (because you can define things on the object-level)?
But yes, this is probably just one step further and not part of this ADR necessarily
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.
Maybe to make it more clear a self-loop with "validation" would help? To be clear what the schemas are even used for in the frontend?
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.
How are going to associate the values and translation from Strapi? Or it's going to be as it's?
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.
Generally I think our data model in strapi also needs to be refactored, but I didn't want to include this in this ADR (and also not create another ADR, we already have a lot...).
So for now I propose to leave as is because each flow page in Strapi already contains the correct labels etc.
But I could be convinced to park this ADR in favor of restructuring our CMS first (I think @rbrtrfl prefers that too)
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.
so should we first restructuring our CMS first before to make the change proposal in this ADR?
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 dont have a strong opinion on the order, however I have the feeling restructuring the CMS is probably going to be a bigger task and I think the benefits of this are useful already.
But I would be fine with either order
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.
Why is called
stepSchema
? Those are not fieldSchema? Not sure why we have decided for the namestep
. It bring a bit of confusion when I need to explain it for other people, specially for non engineers.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.
Its the collection of all
fieldSchemas
for specific step. For Example:startAirport
andendAirport
would each be afieldSchema
andairportsSchema
would be astepSchema
. MaybepageSchema
is a better name?Note: This construct already exists as the return value of
validatorFromFieldnames
but is generated on the fly and therefore quite hiddenThere 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.
For me still not clear how the fieldNames will be connected with CMS. Can you better explain this integration? And where the validation should be set? In the CMS or source 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.
I agree this is confusing because we probably wouldn't send the fieldnames right now (as they already exist on each flow page). The schemas continue to live in the app just like they do now
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.
How this changes would improve the development process?
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 think having an understanding of which page fills out which field is going to be useful when reasoning about flow logic. And creating new flows should be much faster.
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.
Do you mean about the
flow logic
, only if I use the .ts instead of .json for the configuration of the xstate?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, this would apply to any flow. I think this will become more of a problem as we add more flows because its will get harder and harder to remember with field comes from which page
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 am unsure if "App works without CMS" is a good thing. If the app can function without the CMS, does that mean it might not alert us to issues with missing content? Since content is essential to delivering value to users, wouldn’t it be better for the app to indicate when the CMS isn’t working or content is missing? This way, we’d catch potential issues during testing or production instead of relying on users to report them. or maybe I understood it differently?
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.
Of course altering of missing content should still happen. but we shouldn't need to crash the whole application, especially when prototyping. Also the pruner shouldn't need to rely on the CMS