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 :)
| interface FileUploadProps extends HTMLProps<HTMLDivElement> { | ||
| error?: string; | ||
| hint?: string; | ||
| children: React.ReactNode; |
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
| hint, | ||
| children, | ||
| ...rest | ||
| }: FileUploadProps) => { |
There was a problem hiding this comment.
You can remove the : FileUploadProps as the props are already defined on line 10.
| ...rest | ||
| }: FileUploadProps) => { | ||
| return ( | ||
| <div className={`nhsuk-form-group ${error ? 'nhsuk-form-group--error' : ''}`} {...rest}> |
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.
| }: FileUploadProps) => { | ||
| return ( | ||
| <div className={`nhsuk-form-group ${error ? 'nhsuk-form-group--error' : ''}`} {...rest}> | ||
| <label htmlFor="file-upload-label" className="nhsuk-label"> |
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
| {children} | ||
| </label> | ||
| {error && ( | ||
| <span |
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