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

[F] Inline questions #71

Merged
merged 5 commits into from
Nov 1, 2023
Merged

[F] Inline questions #71

merged 5 commits into from
Nov 1, 2023

Conversation

alexgoff
Copy link
Contributor

JIRA: https://jira.lsstcorp.org/browse/EPO-8593

What this change does

Adds client wire up for inline questions, includes new typings for stored answers and some changes to how answers are sent to the server (as JSON)

Notes for reviewers

Use the new database uploaded to dumps on 10/30 and check the inline questions (pg. 10 is a good starting point), and verify them as well in Craft that the entries can be created, saved, and added to pages.

Include an indication of how detailed a review you want on a 1-10 scale.

  • 5 = "Please make sure there are no obvious errors and that you believe it does what it says it does"

Testing

  • Verify inline questions have correct client display on pg. 10.
  • Complete questions, leave the page or investigation, and return. Verify that the questions load with stored answers correctly.
  • Progress to a save point and verify that the saved answers go to the server.

Checklist

If any of the following are true, please check them off

  • This is a breaking change: changes in page data (/src/data/pages/PAGE.json) or scientific data (anything in /static/) and I will notify Product Marketing when it is in prod and user visible.
  • This change impacts several investigations (e.g. the change affects reuseed styles, widgets, pages, QAs, utility methods, etc.)
  • This change is isolated to a specific page or Component (i.e. it's a one-off)
  • I don't know what this change does

Screenshots (optional)

Before:

After:

@alexgoff alexgoff added enhancement New feature or request EPO Issue EPO team is working on labels Oct 30, 2023
@alexgoff alexgoff requested review from blnkt and kylepratuch October 30, 2023 21:35
@alexgoff alexgoff self-assigned this Oct 30, 2023
@@ -35,7 +35,12 @@ export default async function saveAnswers(
return "statusError";
}

const answerSet = Object.values(answers);
const answerSet = Object.values(answers).map(({ data, ...values }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kylepratuch I wanted your input here on stringifying the data property before putting it into the query. We talked about the DB storing all values as a string type, since the typing of data is very loose for the different question types (it can be a string, array of strings, generic object, mildly typed object), I opted to stringify all the data rather than trying to adapt the limited GQL types to fit.

Choose a reason for hiding this comment

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

@alexgoff I'm not experienced enough with typescript to know at a glance if this is going to do what we discussed, but I trust that you do. :) Generally, I don't see an issue consistently storing the Answer data field as strings of JSON since that's really just supposed to consist of the values provided by users. If you start needing to store additional attributes beyond just the given answer values it might be worth adding some additional fields to the Answer element instead.

onChangeCallback={(value: string | string[]) =>
onChangeCallback &&
onChangeCallback(
{ ...(data as InlineQuestionData), [partId]: value },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updates the data object with a value using the inline question part's ID as the key.

}
`;

export const QuestionLabel = styled.div`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

co-locate these styles with the SimpleQuestion component so they don't pollute the inline questions

@blnkt blnkt changed the base branch from EPO-8593 to develop November 1, 2023 16:52
@blnkt blnkt merged commit cc8dd4d into develop Nov 1, 2023
2 checks passed
Copy link

github-actions bot commented Nov 1, 2023

A preview of this PR will be available at https://epo8591-dot-investigations-client-dot-skyviewer.appspot.com until the request is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request EPO Issue EPO team is working on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants