Add tests for molecules input form field#68
Conversation
SaidShah
requested review from
brandongregoryscott,
jstevenson and
wintondeshong
Codecov Report
@@ Coverage Diff @@
## main #68 +/- ##
==========================================
+ Coverage 25.66% 28.93% +3.26%
==========================================
Files 51 51
Lines 900 902 +2
Branches 209 209
==========================================
+ Hits 231 261 +30
+ Misses 662 639 -23
+ Partials 7 2 -5
Continue to review full report at Codecov.
|
| // Arrange | ||
| const label = faker.random.words(); | ||
| const firstErrorMessage = faker.random.words(); | ||
| const secondErrorMessage = faker.random.words(); |
There was a problem hiding this comment.
Nitpick: try to avoid numerical/sequential naming of test variables unless it has a meaning (ie, we are actually expecting firstErrorMessage to be the first result in a returned list). If the order & value do not really matter, this could be wrapped up in an array:
const errorMessages = [faker.random.words(), faker.random.words()];Honestly, this might be a good candidate as a TestUtil method in our JS testing package (I know I've done a similar thing a few places, too)
| import { | ||
| InputFormField, | ||
| InvalidInputFormValueClass, | ||
| ShowLabelForScreenReadersOnlyClass, |
There was a problem hiding this comment.
Could these two classnames be namespaced to the InputFormField component? something a little more descriptive but hopefully not too verbose -
InputFormFieldInvalidClass
InputFormFieldScreenReaderLabelClass
|
|
||
| const COMPONENT_CLASS = "c-form-field"; | ||
| export const InvalidInputFormValueClass = "-invalid"; | ||
| export const ShowLabelForScreenReadersOnlyClass = "sr-only"; |
There was a problem hiding this comment.
Actually, now that I'm seeing this, I think this is a pretty common classname we use for screenreader labels/accessibility. Do a quick search in the codebase, but we could probably promote this classname to a constant outside of this component and share it around
|
@brandongregoryscott - Thanks for the feedback. I have made some changes based on your comments if you would like to take another look. 👍 |
brandongregoryscott
left a comment
brandongregoryscott
left a comment
There was a problem hiding this comment.
@SaidShah Thanks for making that update! Just some final restructuring of where that enum is & it's good to go 👍
| @@ -0,0 +1,3 @@ | |||
| export enum AccessibilityLabels { | |||
There was a problem hiding this comment.
Can this be moved to an enums folder instead of constants? Really nitpicky, but it's a little misleading otherwise.
Additionally, I'd move the folder up a level (not nested under atoms) - seeing as we're already using it in a molecule, it doesn't seem specific to atom components.
3 participants
Closes #24