feat: logic for conditionally rendering feedback UI #2678

Mike-Heneghan · 2024-01-18T14:11:21Z

What

Based on Figma prototype conditionally render the new UI components
Add dummy form submission methods and basic inout validation

Why

Sets up the paths for users to provide feedback
Discrete chunk of work which can be tested / reviewed

Testing

Testing link: https://2678.planx.pizza/testing/feedback-ui/unpublished
As default no change to feedback banner
As default no chance to the more info UI
Toggle the feature window.featureFlags.toggle("SHOW_INTERNAL_FEEDBACK")
Test the feedback banner behaviour works as per Figma Prototype
Test the more info section works as per Figma Prototype

Screen recording

Screen.Recording.2024-01-18.at.15.14.59.mov

github-actions · 2024-01-18T14:35:00Z

Removed vultr server and associated DNS entries

Mike-Heneghan · 2024-01-18T15:32:50Z

editor.planx.uk/src/components/Feedback.tsx

+  );
+  const previousFeedbackView = usePrevious(currentFeedbackView);
+
+  function handleFeedbackViewClick(event: FeedbackViewClickEvents) {


I tried implementing the handler as a single function with a switch statement which I think has worked pretty well?

Agreed - nice and need and maintainable! Take a look at my comment below on how we could make this a little safer.

Mike-Heneghan · 2024-01-18T15:34:12Z

editor.planx.uk/src/components/Feedback.tsx

I feel like there's an opportunity to refactor this file, potentially splitting into more components and reusing more content? Although I don't want to get too bogged down in optimisations I suppose and can always come back to it later?

This might be a nice to have in future I think - for now it feels ok to group them? We can always come back and split up (and maybe write some Storybook stories) and tests sometime soon?

Mike-Heneghan · 2024-01-19T12:30:30Z

Design & implement in-house feedback feature & retire Feedback Fish

ianjon3s · 2024-01-23T10:28:40Z

I've had a click through test of this and it's all working perfectly from a user-perspective!

Mike-Heneghan · 2024-01-23T10:34:35Z

I've had a click through test of this and it's all working perfectly from a user-perspective!

Thanks for checking this Ian! It was really helpful having your prototype for this work 🙌

DafyddLlyr

Super clean and nice code - a pleasure to review!

A few minor comments and suggestions, nothing blocking.

Some small UI / design thoughts (these are for @ianjon3s to think about also...!) -

The white on grey here feels quite hard to look at - is there an alternative or is this ok?
Should we use the ErrorWrapper component over native browser validation (e.g. for required fields)? The benefits would be a more consistent UI and we also now capture analytics via the ErrorWrapper which could be useful to track.

editor.planx.uk/src/ui/public/FeedbackOption.tsx

editor.planx.uk/src/components/Feedback.tsx

DafyddLlyr · 2024-01-23T10:56:20Z

editor.planx.uk/src/components/Feedback.tsx

+    }
+  }
+
+  const handleFeedbackFormSubmit = (e: any) => {


e: any is great for now but once we have a real form (and maybe Formik) wired up we should make sure to type this correctly 👍

DafyddLlyr · 2024-01-23T11:01:14Z

editor.planx.uk/src/components/Feedback.tsx

+    );
+  }
+
+  function Feedback(): FCReturn {


In TypeScript switch statements don't read types as nicely as they should unfortunately.

I'd suggest adding an exhaustive check here as it would throw a type error if we added a new FeedbackView but didn't update this case statement.

Tbh maybe we should enforce this with something like https://typescript-eslint.io/rules/switch-exhaustiveness-check/ to avoid having to repeatedly use an exhausive guard. Maybe a nicer solution!

This is very interesting point and I can definitely imagine lots of situations where the switch isn't updated.

I had a wee look at the link you shared and I think I've added it to our eslint setup here: 426c45c

Seems to be working for me locally.

Thanks for taking a look at this!

Locally on your branch this isn't quite working for me (it's maybe a me issue!).

If I drop the final "thanks" case from this switch locally I can run pnpm check and it picks up an issue with this file -

➜ editor.planx.uk git:(mh/setup-feedback-form-conditional-rendering) pnpm check > editor.planx.uk@ check /Users/dafyddllyr/Code/planx-new/editor.planx.uk > tsc --noEmit && pnpm lint src/components/Feedback.tsx:363:24 - error TS2366: Function lacks ending return statement and return type does not include 'undefined'. 363 function Feedback(): FCReturn { ~~~~~~~~ Found 1 error in src/components/Feedback.tsx:363

However my IDE doesn't pick up the affected line, it just gives an issue right at the top of the affected file -

(The location of the .tsconfig.json should be /Users/dafyddllyr/Code/planx-new/editor.planx.uk/tsconfig.json)

Are you hitting these issues?

This is a bit of a weird one 🤔

Normally I use npx lint-staged before I make a commit and when I remove the thanks case I see:

❯ npx lint-staged ✔ Preparing lint-staged... ❯ Running tasks for staged files... ❯ package.json — 1 file ❯ src/**/*.{js,jsx,ts,tsx} — 1 file ✖ eslint --fix [FAILED] ✔ src/**/*.{js,jsx,ts,tsx,json,css,scss,md,html} — 1 file ❯ src/**/*.{ts,tsx} — 1 file ✖ scripts/tsc-lint.sh [KILLED] ↓ Skipped because of errors from tasks. [SKIPPED] ✔ Reverting to original state because of errors... ✔ Cleaning up temporary files... ✖ eslint --fix: /Users/mike/planx-new/editor.planx.uk/src/components/Feedback.tsx 95:40 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any 98:28 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any 345:12 warning 'ThanksForFeedback' is defined but never used. Allowed unused vars must match /^_/u @typescript-eslint/no-unused-vars 364:13 error Switch is not exhaustive. Cases not matched: "thanks" @typescript-eslint/switch-exhaustiveness-check ✖ 4 problems (1 error, 3 warnings) ✖ scripts/tsc-lint.sh failed without output (KILLED).

When I try pnpm check I see:

Checking formatting... All matched files use Prettier code style! ❯ pnpm check > editor.planx.uk@ check /Users/mike/planx-new/editor.planx.uk > tsc --noEmit && pnpm lint src/components/Feedback.tsx:363:24 - error TS2366: Function lacks ending return statement and return type does not include 'undefined'. 363 function Feedback(): FCReturn { ~~~~~~~~ Found 1 error in src/components/Feedback.tsx:363

In both cases I don't see any warning in my editor 🤔

If you have time maybe we could jump on a huddle at @DafyddLlyr as I'm slightly perplexed by this one. I think my setup/workflow hasn't been quite right.

On main I don't see the FCReturn comment so I've definitely introduced a bug.

Yeah lets do that 👍

Could you please drop these changes from the PR so that we can look at it in isolation, and don't hold up this PR unnecessarily?

I'm also thinking FCReturn might actually be correct as if a case is missing i.e. the currentFeedbackView is "thanks" then the component does indeed lack a return statement and isn't guaranteed to return a react component?

DafyddLlyr · 2024-01-23T11:04:44Z

editor.planx.uk/src/components/Feedback.tsx

+  );
+  const previousFeedbackView = usePrevious(currentFeedbackView);
+
+  function handleFeedbackViewClick(event: FeedbackViewClickEvents) {


Agreed - nice and need and maintainable! Take a look at my comment below on how we could make this a little safer.

editor.planx.uk/src/components/MoreInfoFeedback.tsx

DafyddLlyr · 2024-01-23T11:13:13Z

editor.planx.uk/src/components/Feedback.tsx

This might be a nice to have in future I think - for now it feels ok to group them? We can always come back and split up (and maybe write some Storybook stories) and tests sometime soon?

Mike-Heneghan · 2024-01-24T11:51:58Z

editor.planx.uk/src/components/MoreInfoFeedback.tsx

-    <>
+  type View = "yes/no" | "input" | "thanks";
+
+  type Sentiment = "helpful" | "unhelpful";


I've also tried to reflect the language of the feedback_type_enum here so that it hopefully makes things easier when the forms are wired up.

DafyddLlyr

One small question - might be an issue with my local setup but just wanted to cross-check.

DafyddLlyr · 2024-01-24T12:33:47Z

editor.planx.uk/src/components/Feedback.tsx

+    );
+  }
+
+  function Feedback(): FCReturn {


Thanks for taking a look at this!

Locally on your branch this isn't quite working for me (it's maybe a me issue!).

If I drop the final "thanks" case from this switch locally I can run pnpm check and it picks up an issue with this file -

➜ editor.planx.uk git:(mh/setup-feedback-form-conditional-rendering) pnpm check > editor.planx.uk@ check /Users/dafyddllyr/Code/planx-new/editor.planx.uk > tsc --noEmit && pnpm lint src/components/Feedback.tsx:363:24 - error TS2366: Function lacks ending return statement and return type does not include 'undefined'. 363 function Feedback(): FCReturn { ~~~~~~~~ Found 1 error in src/components/Feedback.tsx:363

However my IDE doesn't pick up the affected line, it just gives an issue right at the top of the affected file -

(The location of the .tsconfig.json should be /Users/dafyddllyr/Code/planx-new/editor.planx.uk/tsconfig.json)

Are you hitting these issues?

- Based on Figma prototype conditionally render the new UI components - Add dummy form submission methods

- As per: https://github.com/theopensystemslab/planx-new/pull/2678/files#r1463088523

- Move from using enums to type with string unions - Remove overuse of 'feedback' from naming

- Move to using onClick so it's implicit rather than needing to be redefined

Mike-Heneghan · 2024-01-24T13:42:33Z

Super clean and nice code - a pleasure to review!

A few minor comments and suggestions, nothing blocking.

Some small UI / design thoughts (these are for @ianjon3s to think about also...!) -

The white on grey here feels quite hard to look at - is there an alternative or is this ok?

Should we use the ErrorWrapper component over native browser validation (e.g. for required fields)? The benefits would be a more consistent UI and we also now capture analytics via the ErrorWrapper which could be useful to track.

There are great spots @DafyddLlyr!

I'll defer to your judgement/update for the "Report an Issue" button @ianjon3s but we can update this in a later PR.

For the ErrorWrapper I think I'll add this when I wire up the forms following the pattern in your work with the design settiings @DafyddLlyr 👍

Mike-Heneghan commented Jan 18, 2024

View reviewed changes

Mike-Heneghan self-assigned this Jan 19, 2024

Mike-Heneghan requested a review from a team January 19, 2024 11:47

Mike-Heneghan marked this pull request as ready for review January 19, 2024 11:47

DafyddLlyr approved these changes Jan 23, 2024

View reviewed changes

Mike-Heneghan commented Jan 24, 2024

View reviewed changes

DafyddLlyr approved these changes Jan 24, 2024

View reviewed changes

Mike-Heneghan added 7 commits January 24, 2024 13:36

feat: logic for conditionally rendering feedback UI

f395563

- Based on Figma prototype conditionally render the new UI components - Add dummy form submission methods

refactor: update rendering logic enums for readability

f52936a

- As per: https://github.com/theopensystemslab/planx-new/pull/2678/files#r1463088523

refactor: move from enums to type with string unions

29fe3e5

refactor: remove unnecessary use of 'Feedback' in sub component naming

30acdf4

chore: update Icon prop case and type

386ae0a

refactor: swap from enum to string unions

d89e3ea

- Move from using enums to type with string unions - Remove overuse of 'feedback' from naming

refactor: rename handleClick prop to use onClick

cef5a41

- Move to using onClick so it's implicit rather than needing to be redefined

Mike-Heneghan force-pushed the mh/setup-feedback-form-conditional-rendering branch from 7d9f353 to cef5a41 Compare January 24, 2024 13:38

DafyddLlyr approved these changes Jan 24, 2024

View reviewed changes

Mike-Heneghan merged commit 185e2a0 into main Jan 24, 2024
12 checks passed

Mike-Heneghan deleted the mh/setup-feedback-form-conditional-rendering branch January 24, 2024 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: logic for conditionally rendering feedback UI #2678

feat: logic for conditionally rendering feedback UI #2678

Mike-Heneghan commented Jan 18, 2024 •

edited

Loading

github-actions bot commented Jan 18, 2024 •

edited

Loading

Mike-Heneghan Jan 18, 2024

DafyddLlyr Jan 23, 2024

Mike-Heneghan Jan 18, 2024

DafyddLlyr Jan 23, 2024

Mike-Heneghan commented Jan 19, 2024

ianjon3s commented Jan 23, 2024

Mike-Heneghan commented Jan 23, 2024

DafyddLlyr left a comment

DafyddLlyr Jan 23, 2024

DafyddLlyr Jan 23, 2024

DafyddLlyr Jan 23, 2024

Mike-Heneghan Jan 24, 2024

DafyddLlyr Jan 24, 2024

Mike-Heneghan Jan 24, 2024 •

edited

Loading

Mike-Heneghan Jan 24, 2024 •

edited

Loading

DafyddLlyr Jan 24, 2024

Mike-Heneghan Jan 24, 2024

DafyddLlyr Jan 23, 2024

DafyddLlyr Jan 23, 2024

Mike-Heneghan Jan 24, 2024

DafyddLlyr left a comment

DafyddLlyr Jan 24, 2024

Mike-Heneghan commented Jan 24, 2024

feat: logic for conditionally rendering feedback UI #2678

feat: logic for conditionally rendering feedback UI #2678

Conversation

Mike-Heneghan commented Jan 18, 2024 • edited Loading

What

Why

Testing

Screen recording

github-actions bot commented Jan 18, 2024 • edited Loading

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

Mike-Heneghan commented Jan 19, 2024

ianjon3s commented Jan 23, 2024

Mike-Heneghan commented Jan 23, 2024

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

Mike-Heneghan Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Mike-Heneghan Jan 24, 2024 • edited Loading

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

DafyddLlyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mike-Heneghan commented Jan 24, 2024

Mike-Heneghan commented Jan 18, 2024 •

edited

Loading

github-actions bot commented Jan 18, 2024 •

edited

Loading

Mike-Heneghan Jan 24, 2024 •

edited

Loading

Mike-Heneghan Jan 24, 2024 •

edited

Loading