[Frontend] Data Upload v1: Playtesting Followups #33
Conversation
mxosman
force-pushed
the
mahmoud/bu-playtest
branch
from
ced86ff
to
8ec0cdd
Compare
mxosman
force-pushed
the
mahmoud/bu-playtest
branch
2 times, most recently
from
52d9ba3
to
b9512d7
Compare
mxosman
changed the title
There was a problem hiding this comment.
@mxosman this looks awesome! Thanks for the fantastic PR summary table with videos for each new piece of behavior.
Besides the naming of the "Invalid Metrics" section, which I commented about inline, I also want to think about the "Missing Metric" warning. It doesn't make sense to say that a metric was upload successfully and then show the "Missing Metric" warning right underneath, does it? Curious to hear from you @mxosman and/or others (cc @nichelle-hall and @terryttsai ) if they have thoughts for how to handle. I think Missing Metrics should stay as warnings, not errors, but maybe they deserve their own section, so it doesn't look like this metric was uploaded sucessfully?
| ))} | ||
| {preIngestErrors && preIngestErrors.length > 0 && ( | ||
| <Message> | ||
| <MetricTitle>Invalid Metrics</MetricTitle> |
There was a problem hiding this comment.
I'm wondering if we should include this header, or if we should call it something else. Invalid metrics (well actually, invalid sheetnames) are, I think, only one type of pre-ingest error. Well, actually @nichelle-hall should confirm -- are there any other types?
If not, we should change the naming around pre-ingest errors and only allow this array to contain invalid sheetname errors, and then change this header to say "Invalid Sheetnames."
If there can be other kinds of pre-ingest errors, we should either get rid of this header or rename it to something like "Miscellaneous Errors".
Curious to hear others' thoughts as well! cc @terryttsai , @nichelle-hall
There was a problem hiding this comment.
I think it'd be simplest to not have a metric title for pre-ingest errors, and for the case of invalid sheetname, just have that error be titled "Invalid Sheetname"
There was a problem hiding this comment.
I only included this header because these errors don't come with a display name - so they just render as sections without a header - this was my attempt to at least consolidating them under one umbrella.
I'm happy with "Invalid Sheetnames" if there are no other types of pre-ingest errors, and "Miscellaneous Errors" for a mix type of pre ingest errors!
@terryttsai - would your suggestion be more changing the backend response on those pre-ingest errors to include a title or something like "Invalid Sheetname"?
There was a problem hiding this comment.
Why don't we do the following: frontend can just change the "Invalid Metrics" title to "Other", this section will contain all pre-ingest errors, and the backend will rename "Metric Not Found" to "Invalid Sheetname" (cc @nichelle-hall ). Wdyt?
There was a problem hiding this comment.
Ooo - I like that idea!
| ))} | ||
| {preIngestErrors && preIngestErrors.length > 0 && ( | ||
| <Message> | ||
| <MetricTitle>Invalid Metrics</MetricTitle> |
There was a problem hiding this comment.
I think it'd be simplest to not have a metric title for pre-ingest errors, and for the case of invalid sheetname, just have that error be titled "Invalid Sheetname"
Thanks, Lili! Yeah - I do agree that it doesn't make sense to display "Missing Metric" if it indeed was not uploaded successfully. Definitely open to thoughts/discussion on this. My focus was separating the errors entirely and rendering success messages for those metrics with only warnings or no sheets/messages at all - I hadn't fully thought about the details of the individual warnings that will come up. It might be helpful too to get an idea of the spectrum of warnings that could happen to come up with an all encompassing solution. |
|
@mxosman re: Missing Metric -- I wonder if the easiest solution is for the backend just to change this to an error, rather than warning state. Then these will appear in the error section and I think it will all make more sense that way. Originally we didn't want to do that because 1) we didn't want to be these errors to be blocking, and 2) we wanted to give agencies to omit metrics they weren't able to report. But 1) is solved by the fact that this page is no longer blocking (you still go to Review page after), and 2) is solved by metric config and context page. Wdyt? cc @nichelle-hall |
That makes sense to me, Lili! |
mxosman
force-pushed
the
mahmoud/bu-playtest
branch
from
60b4ce3
to
c16d5ff
Compare
Extend hitbox for downloading templates to the entire button and not the text Update error page with download example link, extend design-system dropdown, set max height and overflow scroll Update upload loader Refactor error page - initial pass at breaking out the error page Add successes and errors section headers Reverse the loading and animation, replace with loading spinner and static text Fix copy Update comments, change component names, update affected components Fix hasErrorsOrWarnings logic More clean up - add comments Fix warnings, move loading logic positioning, add useEffect to update selected system to fix bug Update copy, update copy for 0 errors, and remove TODO Move error/warning desc to own func Change invalid metric to other, always render continue button fix lint warning Change Continue to Review button
mxosman
force-pushed
the
mahmoud/bu-playtest
branch
from
c16d5ff
to
ef7d7df
Compare
Description of the change
Addresses the following Data Upload (v1) playtesting feedback:
NewErrorWarningPage.mov
NewUploadLoading.mov
MenuDropdownHeightAdjustment.mov
DownloadtextDownloadExampleHitbox.mov
(download example)DownloadExampleInErrorPage.mov
Related issues
Closes #35
Closes #28
Checklists
Development
This box MUST be checked by the submitter prior to merging:
These boxes should be checked by the submitter prior to merging:
Code review
These boxes should be checked by reviewers prior to merging: