Skip to content
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

feat: feedback component submits to feedback table + tidy ups #3900

Merged
merged 45 commits into from
Nov 12, 2024

Conversation

jamdelion
Copy link
Contributor

@jamdelion jamdelion commented Nov 1, 2024

In this PR:

Improvements from comments on previous PR:

  • Fixed more information link not appearing as expect on public component
  • Swapped RichTextInput for TextInput component in public view
  • Restyled disclaimer using shared component
  • Renamed placeholders file given will be used in templates, and made the object typeable.

Also:

  • Fixed styling of face boxes uppercase text.
  • Hook into existing feedback API (added 'feedback score' column to table)
  • Also add _feedback to breadcrumbs on submit

How to test 🧪

  • Turn on the feature flag with `window.featureFlags.toggle("FEEDBACK_COMPONENT").
  • Try to add a Feedback component to a flow, publish it etc!

🎟️ Trello ticket: https://trello.com/c/jEOLoubW/3034-can-we-introduce-in-house-end-of-service-feedback

Todo:

  • e2e testing
  • Add feedback score to feedback log table, and add new feedback type (currently renders as default 'inaccuracy')
  • validation and error handling, with testing
  • use the _feedback key in metabase (add to allow_lists)
  • make sure component works on 'back' - previouslySubmittedProps

Copy link

github-actions bot commented Nov 1, 2024

Removed vultr server and associated DNS entries

@jamdelion jamdelion force-pushed the jh/follow-on-feedback-component branch from 25d8a09 to 47d51bc Compare November 4, 2024 14:55
Copy link

github-actions bot commented Nov 6, 2024

🤖 Hasura Change Summary compared a subset of table metadata including permissions:

Updated Tables (1)

  • public.feedback permissions:

    insert select update delete
    public /
    1 added column permissions
    insert select update
    public ➕ feedback_score

@jamdelion jamdelion force-pushed the jh/follow-on-feedback-component branch from e5f317c to a8380f9 Compare November 7, 2024 14:35
@jamdelion jamdelion marked this pull request as ready for review November 7, 2024 14:59
@jamdelion jamdelion changed the title jh/follow on feedback component feat: feedback component submits to feedback table + tidy ups Nov 7, 2024
@jamdelion jamdelion requested a review from a team November 7, 2024 15:00
Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff - working as expected 👍

Code is clear and well structured, easy to review and follow along.

One thing for a follow up PR is to make sure that the component works on "back" - have a look at how the previouslySubmittedData prop is used in other components.

A few comments below - let me know if you want to talk anything through.

editor.planx.uk/docs/adding-a-new-component.md Outdated Show resolved Hide resolved
editor.planx.uk/src/@planx/components/Feedback/model.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@
alter table "public"."feedback" add column "feedback_score" text
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see previous comment - maybe an integer column with a CHECK constraint (e.g. 1-5) is a good option here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I'm not sure of - can I delete some of these migrations to make the PR cleaner? Or is my back and forth on this immortalised forever more? 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Can manually combine migrations into a single up.sql (note you'll want to destroy Vultr before pushing commit so it rebuilds from scratch to test) or there's a Hasura CLI squash command (I find this command finicky and usually go the manual route tbh!)

Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes & previous feedback all look good here - happy to approve to not block progressing !

@jessicamcinchak jessicamcinchak dismissed DafyddLlyr’s stale review November 12, 2024 11:47

Dismissing "requested changes" while Daf is out sick!

@jamdelion jamdelion merged commit a047f8d into main Nov 12, 2024
12 checks passed
@jamdelion jamdelion deleted the jh/follow-on-feedback-component branch November 12, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants