-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: File Upload #24
base: master
Are you sure you want to change the base?
Conversation
Feature/file upload
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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