Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

feat(appointments) reason field no longer labelled as a required field when making appointments. #2003

Merged
merged 15 commits into from
Apr 23, 2020
Merged

feat(appointments) reason field no longer labelled as a required field when making appointments. #2003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1f19bb8
npm yarn install
JDarke Apr 21, 2020
1d922c7
fix(src/components/input/textfieldwithlabelformgroup.tsx): update Label
JDarke Apr 21, 2020
7efe678
revert(package.json): remove yarn listing
JDarke Apr 21, 2020
c7dba69
Merge branch 'master' into master
Apr 21, 2020
ba91b21
Merge branch 'master' into master
Apr 21, 2020
404a7a2
Merge branch 'master' into master
Apr 22, 2020
6a2523c
Merge branch 'master' into master
Apr 22, 2020
9f8c1d7
Merge branch 'master' into master
Apr 22, 2020
9bfb378
Merge branch 'master' into master
Apr 22, 2020
71cd8b0
Merge branch 'master' into master
Apr 22, 2020
8cac332
Merge branch 'master' into master
Apr 22, 2020
92ad7f7
Merge branch 'master' into master
Apr 22, 2020
b7e355a
Merge branch 'master' into master
Apr 22, 2020
36b889e
test(textfieldwithlabelformgroup.test.tsx): add test to Label isRequired
JDarke Apr 22, 2020
7791d9e
refactor(textfieldwithlabelformgroup.test.tsx): make props semantic
JDarke Apr 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/__tests__/components/input/TextFieldWithLabelFormGroup.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,25 @@ describe('text field with label form group', () => {
expect(label.prop('text')).toEqual(expectedName)
})

it('should render label as required if isRequired is true', () => {
const expectedName = 'test'
const expectedRequired = true
const wrapper = shallow(
<TextFieldWithLabelFormGroup
name={expectedName}
label="test"
value=""
isEditable
isRequired={expectedRequired}
onChange={jest.fn()}
/>,
)

const label = wrapper.find(Label)
expect(label).toHaveLength(1)
expect(label.prop('isRequired')).toBeTruthy()
})

it('should render a text field', () => {
const expectedName = 'test'
const wrapper = shallow(
Expand Down
4 changes: 2 additions & 2 deletions src/components/input/TextFieldWithLabelFormGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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} />
Copy link
Member

Choose a reason for hiding this comment

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

can we add a test that, if isRequired is provided`, that it is passed to the label and marks the label as required

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Screen Shot 2020-04-21 at 18 15 49

Copy link
Member

Choose a reason for hiding this comment

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

The components that you are seeing are coming from this library:
https://github.com/HospitalRun/components

For this test, all we want to verify is that if the isRequired prop is given to the TextFieldWithLabelFormGroup component, that is passes it to the Label component.

It doesn't actually verify that the functionality is working, per se. We trust that the implementation of the Label component works as intended. What we are testing is that we are using the Label component correctly. See this test for how we tested the that the correct text is used:

hospitalrun-frontend/src/__tests__/components/input/TextFieldWithLabelFormGroup.test.tsx

Line 7 in c703928

describe('text field with label form group', () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see...yup, I'll give it a go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

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

Either! I don't have a preference

Copy link
Contributor Author

@JDarke JDarke Apr 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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:

error: You have not concluded your merge (MERGE_HEAD exists).
hint: Please, commit your changes before merging.
fatal: Exiting because of unfinished merge.

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?

Copy link
Contributor Author

@JDarke JDarke Apr 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Here's the test, btw:

describe('text field with label form group', () => {
  describe('layout', () => {
    it('should render a label', () => {
      const expectedName = 'test'
      const wrapper = shallow(
        <TextFieldWithLabelFormGroup
          name={expectedName}
          label="test"
          value=""
          isEditable
          onChange={jest.fn()}
        />,
      )

      const label = wrapper.find(Label)
      expect(label).toHaveLength(1)
      expect(label.prop('htmlFor')).toEqual(`${expectedName}TextField`)
      expect(label.prop('text')).toEqual(expectedName)
    })

   it('should render label as required if isRequired is true', () => {
      const expectedName = 'test'
      const expectedRequired = true
      const wrapper = shallow(
        <TextFieldWithLabelFormGroup
          name={expectedName}
          label="test"
          value=""
          isEditable
          isRequired={expectedRequired}
          onChange={jest.fn()}
        />,
      )

      const label = wrapper.find(Label)
      expect(label).toHaveLength(1)
      expect(label.prop('isRequired')).toBeTruthy()
    })

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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}
Expand Down