feat: add a flow description settings section #3954
Conversation
RODO94
force-pushed
the
rory/templates-description-settings
branch
from
66aaf4e to
17f1f4e
Compare
|
Removed vultr server and associated DNS entries |
RODO94
force-pushed
the
rory/templates-description-settings
branch
from
d336a51 to
da12459
Compare
...src/pages/FlowEditor/components/Settings/ServiceSettings/FlowDescription/FlowDescription.tsx
Show resolved
Hide resolved
| return Boolean(result?.id); | ||
| }, | ||
|
|
||
| getFlowSettings: async (flowSlug, teamSlug) => { |
There was a problem hiding this comment.
I found the naming here is a little unclear / unexpected - we already have a flowSettings property and this is not a getter for that property, but rather a function which fetches settings, status and description of a flow.
I think I'd stick to the existing pattern of -
- Fetch within the route
- Set store variables
- Child components read from stores
Always using the route req object over store values also ensures we're always fetching the right data and there's no room for any race condition or unexpected behaviour. This relies on a user always navigating via a route which sets these.
There was a problem hiding this comment.
Screen.Recording.2024-11-18.at.16.03.49.mov
There was a problem hiding this comment.
Yeah good thinking, I did keep the function name the same for consistency, but probably need some altered naming here. It is complicated a bit by the current flowSettings not really being all the settings for a flow, and rather an object of footer, privacy, and help rich text info. I'll look to update the naming to make it clearer
The req bit is a great catch, sorry for missing this. Added the change now.
Would there be a reason why these settings maintain a different pattern to General Settings and Design Settings where we fetch within the index.tsx of the component?
Think I've fallen into a trap of trying to partially improve code while trying to debug and creating wee errors, thanks for all the help and time
There was a problem hiding this comment.
Would there be a reason why these settings maintain a different pattern to General Settings and Design Settings where we fetch within the index.tsx of the component?
Honestly, I think it's just history! This has been around for a while and we've not yet taken the dive to standardise and update it to match the rest - not a pressing concern right now, but would be a super nice tidy up in future.
Think I've fallen into a trap of trying to partially improve code while trying to debug and creating wee errors, thanks for all the help and time
Very easily done - especially in sections of the code like this which are too low on test coverage.
| @@ -1,12 +1,11 @@ | |||
| import Box from "@mui/material/Box"; | |||
| import Button from "@mui/material/Button"; | |||
| import { formControlLabelClasses } from "@mui/material/FormControlLabel"; | |||
| // eslint-disable-next-line no-restricted-imports | |||
| return Boolean(result?.id); | ||
| }, | ||
|
|
||
| getFlowSettings: async (flowSlug, teamSlug) => { |
There was a problem hiding this comment.
Would there be a reason why these settings maintain a different pattern to General Settings and Design Settings where we fetch within the index.tsx of the component?
Honestly, I think it's just history! This has been around for a while and we've not yet taken the dive to standardise and update it to match the rest - not a pressing concern right now, but would be a super nice tidy up in future.
Think I've fallen into a trap of trying to partially improve code while trying to debug and creating wee errors, thanks for all the help and time
Very easily done - especially in sections of the code like this which are too low on test coverage.
What does this PR do?
This PR introduces a new section in the Service Settings page of the Editor. This houses the flow
descriptionwhich was added to theflowstable in a previous PR: #3945I have created relevant mutations and updated current queries to update, create, and select
descriptioninplanx-corein this PR: theopensystemslab/planx-core#564This new section has been added to the bottom of the Service Settings page as not to disrupt the UI too much, although it may be worth reviewing how these things are structured, but I imagined this is most aligned with the "Rethinking the Editor" work rather than this wee PR.
I've called the setting section "Service Information" in case we want to put anything else in there in the future, type of service, creator, templated dependency etc... and it also allowed me to stop constantly using the word "description"
Note
The input structure follows the pattern of going for an Input Label over a placeholder due to the accessibility needs, I'll add a ticket to update the rest of the settings (Footer / Disclaimer) in line with this