Conversation
Feature/file upload
Tomdango
left a comment
Tomdango
left a comment
There was a problem hiding this comment.
A few minor comments, nothing major - looks good :)
There was a problem hiding this comment.
You don't need to pass in children as it's own prop - the React.FC generic automatically adds it into the definition
There was a problem hiding this comment.
You can remove the : FileUploadProps as the props are already defined on line 10.
There was a problem hiding this comment.
Generally, I prefer using the classNames library for this - it's much neater than having to deal with template literals, and it doesn't result in any extra whitespace.
There was a problem hiding this comment.
You don't really want to use predefined unchangeable IDs in components, so I'd rather you pass in htmlFor={id} and pass the IDs to the correct elements
There was a problem hiding this comment.
The Hint component is already imported - you could directly use an ErrorMessage component here and a Label component further up.
| }); | ||
| it('With Error', () => { | ||
| const component = shallow(<FileUpload error="something wrong">Upload</FileUpload>); | ||
| expect(component).toMatchSnapshot(); |
There was a problem hiding this comment.
I'd want a couple more tests just to check that certain parts of the conditional rendering is working properly - i.e. check for a span.nhsuk-error-message to exist etc.
|
@Tomdango Sorry for the delay, think if addressed all comments :) |
|
Hey @Tomdango possible for some movement on this? |
|
@Tomdango ping. What's the latest with this? Are we in good shape for merging? /cc @KaiSpencer |
3 participants
This adds a File Upload component used in DPS Portal, based on GOV UK.
Inspiration:
https://design-system.service.gov.uk/components/file-upload/
Used in the interim awaiting:
nhsuk/nhsuk-service-manual-community-backlog#93