-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(appointments) reason field no longer labelled as a required field when making appointments. #2003
feat(appointments) reason field no longer labelled as a required field when making appointments. #2003
Changes from 2 commits
1f19bb8
1d922c7
7efe678
c7dba69
ba91b21
404a7a2
6a2523c
9f8c1d7
9bfb378
71cd8b0
8cac332
92ad7f7
b7e355a
36b889e
7791d9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -14,11 +14,11 @@ interface Props { | |||
} | ||||
|
||||
const TextFieldWithLabelFormGroup = (props: Props) => { | ||||
const { value, label, name, isEditable, isInvalid, feedback, onChange } = props | ||||
const { value, label, name, isEditable, isInvalid, isRequired, feedback, onChange } = props | ||||
const id = `${name}TextField` | ||||
return ( | ||||
<div className="form-group"> | ||||
<Label text={label} htmlFor={id} isRequired /> | ||||
<Label text={label} htmlFor={id} isRequired={isRequired} /> | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add a test that, if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I can see, that's tested when FormLabel is imported into Label.tsx...see the attached screenshot (it seemed more useful to use a pic in this instance than to just post the code - apologies if that's incorrect practice!) The /src/components path contains a lot more subfolders than my local clone, so I can't access a local copy of Label.tsx. Is that being compiled on the server side? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The components that you are seeing are coming from this library: For this test, all we want to verify is that if the It doesn't actually verify that the functionality is working, per se. We trust that the implementation of the hospitalrun-frontend/src/__tests__/components/input/TextFieldWithLabelFormGroup.test.tsx Line 7 in c703928
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see...yup, I'll give it a go. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the test to be added into TextFieldWithLabelFormGroup.test.tsx (with the text field test suite you referenced) or should it be a separate local test in TextFieldWithLabelFormGroup.tsx? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either! I don't have a preference There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've got a new commit to push, but it's telling me that my local and remote have diverged. Tried a pull to merge them, and my terminal locked up. Restarted and retried, and now I have this message in terminal:
Is this fixable, or do I need to save my local changes, re-clone the repo to local, and then send a whole new PR, or does it need to be connected to this thread's PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's the test, btw:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I seem to have got it pushed now, and it looks like the earlier ones did, too. Apologies if I'm cluttering up this PR with multiple commits. |
||||
<TextField | ||||
rows={4} | ||||
value={value} | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, this can be removed. Yarn is expected to be globally installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll take that out in the next commit.