Skip to content

Refactor FormSteps message handling to work with generic error#6176

Merged
aduth merged 2 commits intomainfrom
aduth-form-error-map
Apr 8, 2022
Merged

Refactor FormSteps message handling to work with generic error#6176
aduth merged 2 commits intomainfrom
aduth-form-error-map

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Apr 7, 2022

Why: Since we plan to use FormSteps outside the context of document capture for FlowStateMachine v2, it shouldn't be so strongly aware of specific error types (via FormErrorMessage). The previous implementation was also largely motivated by the inability to translate strings outside the context of a React context, which is no longer the case.

@aduth aduth force-pushed the aduth-ts-form-steps branch 3 times, most recently from 0545e8f to b6cc6c7 Compare April 7, 2022 15:26
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 27 to 29
Copy link
Contributor

Choose a reason for hiding this comment

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

the ending backtick on other line is throwing me... would this be approximate the same if we did:

Suggested change
message = `${t('doc_auth.errors.upload_error')} ${t('errors.messages.try_again')
.split(' ')
.join(NBSP_UNICODE)}`;
message = `${t('doc_auth.errors.upload_error')} ${t('errors.messages.try_again')}`
.split(' ')
.join(NBSP_UNICODE);

Copy link
Contributor Author

@aduth aduth Apr 7, 2022

Choose a reason for hiding this comment

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

would this be approximate the same if we did:

No, since that would create non-breaking spaces between every word in the error message. The idea with the NBSP is to make sure that "Please try again" stays or breaks onto a new line, but importantly it stays together (see before/after screenshots in #4773). Applying it to the whole string would probably prevent any line breaks altogether.

Maybe what we could do is something like:

Suggested change
message = `${t('doc_auth.errors.upload_error')} ${t('errors.messages.try_again')
.split(' ')
.join(NBSP_UNICODE)}`;
const tryAgain = t('errors.messages.try_again').split(' ').join(NBSP_UNICODE);
message = `${t('doc_auth.errors.upload_error')} ${tryAgain}`;

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! Yes, this new option makes a lot more sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went a slightly different direction in c0102bf, but same effect of keeping the template string on a single line.

Base automatically changed from aduth-ts-form-steps to main April 7, 2022 17:00
@aduth aduth force-pushed the aduth-form-error-map branch from f039def to 2debca9 Compare April 7, 2022 17:25
aduth added 2 commits April 8, 2022 08:36
**Why**: Since we plan to use FormSteps outside the context of document capture for FlowStateMachine v2, it shouldn't be so strongly aware of specific error types (via FormErrorMessage). The previous implementation was also largely motivated by the inability to translate strings outside the context of a React context, which is no longer the case.

[skip changelog]
@aduth aduth force-pushed the aduth-form-error-map branch from c0102bf to 38e5bce Compare April 8, 2022 12:36
@aduth aduth merged commit f2408d9 into main Apr 8, 2022
@aduth aduth deleted the aduth-form-error-map branch April 8, 2022 13: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.

2 participants