feat: allow options to assign many flags #3820

jessicamcinchak · 2024-10-16T21:19:52Z

Following on from #3764

Gist:

Question & Checklist "Options" currently support a single data field val and single flag each - as we think about the future of automations & filtering, there's actually no reason a single Option shouldn't be able to support multiple flags
- For example, answering "No" might flag up "Planning permission needed" AND "Listed building consent required" and it's ideal for editors to be able to set both in a single go (each Options still supports a single data field val)
Editors should ideally only select up to one flag per category
- In a followup PR we could build nice validation error handling for this
- But the cool thing is that it's actually harmless if they select more than one in the same category, it will simply be treated like flags within the same category are always/already treated - only the left-most, highest order flag matters!

Changes:

Update Option node data type flag from string to Array<Flag["value"]>
Implement MultipleSelect with grouping by category for editor "Option" modals within Questions & Checklists
Update graph node styling with color band per flag
Update preview store methods & associated tests
- autoAnswerableFlag for Filter node navigation
- getResultData/resultData for Result node navigation and analytics tracking
- collectedFlags (no component or navigation dependencies)
Expose collectedFlags in editor Console side panel

Open question:
** All of the above work with existing Option's flag (string) and new/updated Option's flag (string[]) without requiring a content migration of existing flow content (or all existing mock test data!).

But are we happy with slightly semantically incorrect type "flag" for a list of values?
Do we still want to consider a content migration script across ALL flow's to rename flag to flags and format legacy values as nice-to-have? (Might be good practice for clones?)

…ss/select-many-flags

…components

github-actions · 2024-10-17T21:05:04Z

Removed vultr server and associated DNS entries

jessicamcinchak · 2024-10-30T14:45:14Z

Try refactoring the Filter component to be a Question that auto-answers based on flag, rather than passort variable

jessicamcinchak · 2024-10-30T14:45:36Z

Collect flags in the passport

jessicamcinchak · 2024-10-30T22:14:46Z

editor.planx.uk/src/pages/FlowEditor/lib/store/preview.ts

@@ -382,7 +374,74 @@ export const previewStore: StateCreator<

  resultData(flagSet, overrides) {
    const { breadcrumbs, flow } = get();
-    return getResultData(breadcrumbs, flow, flagSet, overrides);


Decided to simplify this into a single function rather than this previous pattern of resultData calling getResultData - cleaning up another reduce along the way 🧹 (we aren't even dealing with an array of anything in this method because a single flagset category is passed in, so reduce was especially irrelevant in this case!! 🙈)

jessicamcinchak · 2024-10-30T22:18:13Z

editor.planx.uk/src/pages/FlowEditor/lib/store/preview.ts

@@ -885,3 +871,43 @@ export const removeNodesDependentOnPassport = (
  }, [] as string[]);
  return { removedNodeIds, breadcrumbsWithoutPassportData: newBreadcrumbs };
 };
+
+const collectedFlagValuesByCategory = (


No logical change here - this has simply been extracted out of autoAnswerableFlag so we can re-use it in collectedFlags and resultData too

…ss/select-many-flags

jessicamcinchak · 2024-10-31T15:52:10Z

Logic followups

jamdelion · 2024-11-01T10:26:33Z

editor.planx.uk/src/@planx/components/Checklist/Editor.tsx

-                      count: gr.children.length,
-                    })),
-                  }
+                  categories: groupedOptions.map((gr) => ({


nit: what do you think about changing these acronyms (gr) to something a bit more meaningful?

I just find it slightly increases the mental load unnecessarily to read something that isn't immediately obvious (like I understood it was a group on second reading, but I think making code as frictionless and easy to read as possible is something to strive for!)

Yep, agree here 👍

Another nice pattern can be to just destructure the keys we need.

I appreciate though that this is just a linting change and not something you've specifically touched so it can come as a follow up if needed.

Suggested change

categories: groupedOptions.map((gr) => ({

categories: groupedOptions.map(({ title, children }) => ({

Fully agree - updated now !

jamdelion · 2024-11-01T10:36:50Z

editor.planx.uk/src/pages/FlowEditor/components/Flow/components/Option.tsx

@@ -36,7 +46,7 @@ const Option: React.FC<any> = (props) => {
            imageAltText={props.data.text}
          />
        )}
-        <div className="band" style={{ background, color }}></div>
+        {flags ? flags.map((flag) => <FlagBand key={`${props.id}-${flag.value}`} flag={flag} />) : <NoFlagBand />}


Nice clear component names 😍

jamdelion

I know Daf said he would review this too but I felt nosy 😆 Looks good to me but obviously feel free to wait for his review as he probably has more context!

…ss/select-many-flags

DafyddLlyr

Working as expected, nice and clear to follow 👍

A few minor suggestions, no showstoppers.

On the whole flag/flags thing I think we can live with this as is for now (flag pointing to an array) but it would be nice to handle a migration here for sure - it's nice simple use case as well.

One other comment would be on the validation - I think it's worth making the effort to enforce one per category just from the point of view of explaining this to users. I think it could be as simple as disabling the renderOption if flag from the same category is already in the group. We could also handle this in the validation schema. I'm aware that August flagged this as a blocker so happy for this to be a follow up here for sure 👍

DafyddLlyr · 2024-11-01T10:51:53Z

editor.planx.uk/src/@planx/components/Checklist/Editor.tsx

-                      count: gr.children.length,
-                    })),
-                  }
+                  categories: groupedOptions.map((gr) => ({


Yep, agree here 👍

Another nice pattern can be to just destructure the keys we need.

I appreciate though that this is just a linting change and not something you've specifically touched so it can come as a follow up if needed.

Suggested change

categories: groupedOptions.map((gr) => ({

categories: groupedOptions.map(({ title, children }) => ({

DafyddLlyr · 2024-11-01T10:55:34Z

editor.planx.uk/src/@planx/components/shared/FlagsSelect.tsx

+import { CustomCheckbox, SelectMultiple } from "ui/shared/SelectMultiple";
+
+interface Props {
+  value?: Array<Flag["value"]>;


Is this actually an optional prop?

Annoying yes because Flag["value"] is optional, despite it always be populated/required in practice. Will try to do a more thorough revisit of types as a followup 👍

DafyddLlyr · 2024-11-01T10:58:06Z

editor.planx.uk/src/@planx/components/shared/FlagsSelect.tsx

+  const value: Flag[] | undefined = useMemo(
+    () =>
+      initialFlagValues?.flatMap((initialFlagValue) => flatFlags.filter((flag) => flag.value === initialFlagValue)),
+    [initialFlagValues, flatFlags],


I think that flatFlags is a totally static value which won't change, so it can be removed from here.

DafyddLlyr · 2024-11-01T10:59:30Z

editor.planx.uk/src/@planx/components/shared/index.ts

@@ -28,7 +28,7 @@ export interface Option {
  id: string;
  data: {
    description?: string;
-    flag?: string;
+    flag?: Array<Flag["value"]>;


nit: Maybe a FlagValues type could be nice?

jessicamcinchak added 5 commits October 16, 2024 12:36

wip

56c4b2a

Merge branch 'main' of github.com:theopensystemslab/planx-new into je…

e2bffcb

…ss/select-many-flags

select multiple working

ff08bfa

Merge branch 'main' of github.com:theopensystemslab/planx-new into je…

ba4be9b

…ss/select-many-flags

use shared multiselect component

32da626

jessicamcinchak mentioned this pull request Oct 17, 2024

feat: auto-answer one at a time, not into the future #3764

Merged

19 tasks

jessicamcinchak added 2 commits October 17, 2024 20:46

Merge branch 'main' of github.com:theopensystemslab/planx-new into je…

3fa6d50

…ss/select-many-flags

accurate types, working selects & graph styling for existing and new …

85e888a

…components

jessicamcinchak added 4 commits October 30, 2024 10:04

rebase

c74de6d

filters work for existing & updated nodes

8f51e6f

Results work

8e2678e

ensure result reasons are correctly hidden/shown

b12deb0

jessicamcinchak added 2 commits October 30, 2024 21:32

fix graph styling

3c9f91d

refactor resultData & collectedFlags, show flags in console

ccf4fe2

jessicamcinchak commented Oct 30, 2024

View reviewed changes

jessicamcinchak added 3 commits October 30, 2024 23:26

update resultData tests

3efb9d4

test suite for collectedFlags

181ff4d

test suite for resultData

96a0dc4

jessicamcinchak marked this pull request as ready for review October 31, 2024 15:33

jessicamcinchak requested a review from a team October 31, 2024 15:33

Merge branch 'main' of github.com:theopensystemslab/planx-new into je…

2923971

…ss/select-many-flags

jamdelion reviewed Nov 1, 2024

View reviewed changes

jamdelion approved these changes Nov 1, 2024

View reviewed changes

Merge branch 'main' of github.com:theopensystemslab/planx-new into je…

ab7f080

…ss/select-many-flags

clearer iterator names

c4d00cb

DafyddLlyr approved these changes Nov 1, 2024

View reviewed changes

remove unnecessary clause from FlagsSelect useMemo

290677f

jessicamcinchak merged commit 8c5f127 into main Nov 1, 2024
11 of 12 checks passed

jessicamcinchak deleted the jess/select-many-flags branch November 1, 2024 11:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow options to assign many flags #3820

feat: allow options to assign many flags #3820

jessicamcinchak commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

jessicamcinchak commented Oct 30, 2024

jessicamcinchak commented Oct 30, 2024

jessicamcinchak Oct 30, 2024 •

edited

Loading

jessicamcinchak Oct 30, 2024

jessicamcinchak commented Oct 31, 2024

jamdelion Nov 1, 2024

DafyddLlyr Nov 1, 2024

jessicamcinchak Nov 1, 2024

jamdelion Nov 1, 2024

jamdelion left a comment

DafyddLlyr left a comment

DafyddLlyr Nov 1, 2024

DafyddLlyr Nov 1, 2024

jessicamcinchak Nov 1, 2024

DafyddLlyr Nov 1, 2024

DafyddLlyr Nov 1, 2024

	categories: groupedOptions.map((gr) => ({
	categories: groupedOptions.map(({ title, children }) => ({

feat: allow options to assign many flags #3820

feat: allow options to assign many flags #3820

Conversation

jessicamcinchak commented Oct 16, 2024 • edited Loading

github-actions bot commented Oct 17, 2024 • edited Loading

jessicamcinchak commented Oct 30, 2024

jessicamcinchak commented Oct 30, 2024

jessicamcinchak Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicamcinchak commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamdelion left a comment

Choose a reason for hiding this comment

DafyddLlyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicamcinchak commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

jessicamcinchak Oct 30, 2024 •

edited

Loading