refactor: checklist component subfolders #4038
Conversation
There was a problem hiding this comment.
Simply moved within an Editor subfolder to be co-located with related components
editor.planx.uk/src/@planx/components/Checklist/Editor/components/Options.tsx
Outdated
Show resolved
Hide resolved
| @@ -58,20 +49,6 @@ const ChecklistComponent: React.FC<Props> = (props) => { | |||
| return <VisibleChecklist {...props} />; | |||
| }; | |||
|
|
|||
| // An auto-answered Checklist won't be seen by the user, but still leaves a breadcrumb | |||
| const AutoAnsweredChecklist: React.FC<Props & { answerIds: string[] }> = ( | |||
There was a problem hiding this comment.
Moved to Public/components to clean-up the main component file a little bit
| function toggleInArray<T>(value: T, arr: Array<T>): Array<T> { | ||
| return arr.includes(value) | ||
| ? arr.filter((val) => val !== value) | ||
| : [...arr, value]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Moved to a helpers file
| const newCheckedIds = formik.values.checked.includes(id) | ||
| ? formik.values.checked.filter((x) => x !== id) | ||
| : [...formik.values.checked, id]; |
There was a problem hiding this comment.
I have a plugin that recommends cleaner ways to write code, it especially loves to refactor if...else blocks!
There was a problem hiding this comment.
Are you satisfied with the readability outcomes of this though? (Honestly no wrong answer!)
Just double-checking that refactors are prioritising the preferences of our team made up of very real human developers and not blindly defaulting to what extensions/ai suggest 🙂 (in my opinion that's how we've inherited sometimes very opaque store methods heavy on reduce with too-minimalist variable names)
There was a problem hiding this comment.
I think switching the if...else to a ternary does not make it less readable imo :) sorry I made it sound like I was just letting the plugin run wild on its own (I definitely ignore a lot of what it says). I mainly made this change to reduce the number of lines of code without making any harder to understand. 🤞
| > | ||
| <ImageButton | ||
| title={option.data.text} | ||
| {options?.map((option) => |
There was a problem hiding this comment.
The plugin just recommended optional chaining instead of options &&...
| function getInitialExpandedGroups() { | ||
| return (groupedOptions ?? ([] as Group<Option>[])).reduce( | ||
| (acc, group, index) => | ||
| groupHasOptionSelected(group, previouslySubmittedData?.answers ?? []) | ||
| ? [...acc, index] | ||
| : acc, | ||
| [] as number[], | ||
| ); | ||
|
|
||
| function groupHasOptionSelected(group: Group<Option>, answers: string[]) { | ||
| return group.children.some((child) => | ||
| answers.some((id) => child.id === id), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Moved to the helpers file
| @@ -201,7 +85,7 @@ describe("Checklist Component - Grouped Layout", () => { | |||
| groupedOptions={groupedOptions} | |||
| />, | |||
| ); | |||
| const [section1Button, section2Button, section3Button, _continueButton] = | |||
There was a problem hiding this comment.
_continueButton was unused
|
Removed vultr server and associated DNS entries |
editor.planx.uk/src/@planx/components/Checklist/Editor/components/Options.tsx
Outdated
Show resolved
Hide resolved
| const newCheckedIds = formik.values.checked.includes(id) | ||
| ? formik.values.checked.filter((x) => x !== id) | ||
| : [...formik.values.checked, id]; |
There was a problem hiding this comment.
Are you satisfied with the readability outcomes of this though? (Honestly no wrong answer!)
Just double-checking that refactors are prioritising the preferences of our team made up of very real human developers and not blindly defaulting to what extensions/ai suggest 🙂 (in my opinion that's how we've inherited sometimes very opaque store methods heavy on reduce with too-minimalist variable names)
|
Thanks for updates here - still getting ShareDB issues on pizza though when trying to connect to flows - Vultr might need a destroy/rebuild? |
|
@jessicamcinchak the pizza is working for me now, hopefully good for you too? 🤞 |
There was a problem hiding this comment.
All working as expected on pizza , thanks !
Spotted one bug (not introduced here) though just to keep on your radar as you're working with this component next few days (ideally can get fixed-forward in a future PR):
- When "Expandable" is toggled on and you set a data field, the validation to ensure that at least one option also sets a data field fails and prevents creating the component (it's fine if options are not grouped into expandable sections). Hasn't been raised externally yet, but definitely a bug on staging/prod too !
Just another classic PR from me! 😅 I was beginning to look at the 'or' option in checklists ticket and started off with a minor refactor to help orient myself in the folder, which rapidly turned into a 600 line PR 🙈
Mostly this is moving stuff into sub-folders to reduce the size of the massively long files!
Here's what the file structure looks like now:
