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

[JDarke]

Fixes #1997 .

Changes proposed in this pull request:

changed line 21 of src/components/input/textfieldwithlabelformgroup.tsx , so that Label attr 'isRequired' now takes value from Props.isRequired? (line 11).

[jsf-clabot]

All committers have signed the CLA.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/hospitalrun/hospitalrun-frontend/2pxqe7pqh
✅ Preview: https://hospitalrun-frontend-git-fork-jdarke-master.hospitalrun.now.sh

[jackcmeyer]

You are right, this can be removed. Yarn is expected to be globally installed.

[JDarke]

Ok, I'll take that out in the next commit.

[jackcmeyer]

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

[JDarke]

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?

[jackcmeyer]

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', () => {

[JDarke]

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

[JDarke]

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?

[jackcmeyer]

Either! I don't have a preference

[JDarke]

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?

[JDarke]

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()
    })

[JDarke]

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.