implements the income question change in the demographic survey #555
Conversation
hamilton
changed the title
knowtheory
changed the title
There was a problem hiding this comment.
Just nits, looks OK to me. I'm concerned that we need to ratchet our eslint rules up a bit but I will tackle that in a separate issue (we already have one where vscode isn't aligning to what our CI is doing based on the same eslint config I'd like to figure out)
hamilton
force-pushed
the
income-question-change
branch
from
2f64bff to
c639f34
Compare
…ts into the right response format and vice-versa
| $: if ($store && $store.demographicsData) { | ||
| intermediateResults = formatAnswersForDisplay(schema, { ...$store.demographicsData }, inputFormatters); | ||
| } | ||
| $: console.log(intermediateResults) |
There was a problem hiding this comment.
Did you mean to leave this?
| @@ -0,0 +1,161 @@ | |||
| /* This Source Code Form is subject to the terms of the Mozilla Public | |||
There was a problem hiding this comment.
I am not necessarily opposed to it, but why should this live here vs. in the svelte tests?
|
|
||
|
|
||
|
|
||
| /** This Svelte action will format an input element on input, focus, and blur events. |
There was a problem hiding this comment.
Nit, but why does it only format on these events, and not as the user types? Wondering if that is a stylistic choice or just pragmatic. I can see the argument for not modifying input while the user is interacting.
There was a problem hiding this comment.
I believe input is when it formats (which is when the user types / copy-pastes). @changecourse's requirements were to prevent inputs that shouldn't exist (like letters in a number field) instead of showing an error on incorrect input. I tried breaking the experience implemented here in various ways but was unsuccessful. This ultimately felt like the best way to control the nature of the inputs appropriate for (1) the form version of the answers (what should the entered-in format look like when the input field is not active, e.g. $100,000 is what we see vs 100000 or 100000), (2) the edit version of the answers (we don't want the currency fields while typing to have the dollar sign and commas), and (3) the response version of the answers (what should the data look like when it goes to the servers?) These three things may all be different, hence this formatter.
| @@ -1,7 +1,9 @@ | |||
| /* This Source Code Form is subject to the terms of the Mozilla Public | |||
There was a problem hiding this comment.
Is there a PR up for the schema change ? It's nice to have it split out to its own JSON, how easily can we compare it to what's actually loaded on the server side?
I'm presuming that it is https://github.com/mozilla-services/mozilla-pipeline-schemas/blob/main/schemas/pioneer-core/demographic-survey/demographic-survey.1.schema.json, in which case I guess it's not trivially comparable :) Maybe something to consider for the future.
There was a problem hiding this comment.
I agree. There is not a PR out and this schema is really just for the purposes of formatting (probably the wrong wording here). This part of the code base will probably be refactored when a real frontend engineer takes over this part of the codebase. It's not the most confusing setup ever, but it was slapped together pretty quickly and it'd be better to use an actual json schema approach or whatever.
closes #496. We'll hold on this until (1) we can get the product owner to give a thumbs up and (2) we have a PR up for the schema.
A demo of this is viewable here.
Checklist for reviewer:
CHANGELOG.mdentry for any non-test change.