Skip to content

LG-4876 Adding the new doc capture error warning view#5593

Merged
amathews-fs merged 20 commits intomainfrom
LG-4876_amathews_doc_capture_error_view
Nov 16, 2021
Merged

LG-4876 Adding the new doc capture error warning view#5593
amathews-fs merged 20 commits intomainfrom
LG-4876_amathews_doc_capture_error_view

Conversation

@amathews-fs
Copy link
Contributor

@amathews-fs amathews-fs commented Nov 8, 2021

Adding the new doc capture error warning view after failure but before review step.

Copy link
Contributor

Choose a reason for hiding this comment

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

we've been naming these files kebab-case, can we rename?

Suggested change
import withProps from '../higher-order/withProps';
import withProps from '../higher-order/with-props';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looking solid so far 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

The parameter here is used for location=, so should be unique. It can be whatever you want to call it, e.g.

Suggested change
url: getFailureToProofURL('capture_tips'),
url: getFailureToProofURL('post_submission'),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. That was a copy and paste that I forgot to fix. Thanks for catching that.

Copy link
Contributor

@aduth aduth Nov 8, 2021

Choose a reason for hiding this comment

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

This should be lowercase, since the capitalized Number refers to a different thing.

See: https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#number-string-boolean-symbol-and-object

Suggested change
* @prop {Number=} remaining_attempts Number of remaining doc capture attempts for user.
* @prop {number=} remaining_attempts Number of remaining doc capture attempts for user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damned, I even looked that up and was surprised to find it had a primitive vs. Object number. I thought about fixing it but apparently never did.

@amathews-fs amathews-fs force-pushed the LG-4876_amathews_doc_capture_error_view branch from aae3d48 to 5e63b6f Compare November 15, 2021 21:39
@amathews-fs amathews-fs marked this pull request as ready for review November 15, 2021 21:41
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks good 👍

{!!unknownFieldErrors &&
unknownFieldErrors.map(({ error }) => <p key={error.message}>{error.message}</p>)}

{remainingAttempts <= 3 && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Might be good idea to pull out this magic number as a constant toward the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good call. I thought about making it a config but then decided against it and just didn't think about pulling it out as a constant.

const steps = [STEPS[1]];

const { getByRole } = render(<FormSteps steps={steps} />);
const button = getByRole('button', { name: 'Create Step Error' });
Copy link
Contributor

Choose a reason for hiding this comment

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

The corresponding button in the STEPS example content existed only for this test case, so could be removed as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I'll remove it.

@amathews-fs amathews-fs merged commit 6f20c9a into main Nov 16, 2021
@amathews-fs amathews-fs deleted the LG-4876_amathews_doc_capture_error_view branch November 16, 2021 16:45
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.

3 participants