Skip to content
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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

KaiSpencer
Copy link

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

Copy link
Collaborator

@Tomdango Tomdango left a 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;
Copy link
Collaborator

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) => {
Copy link
Collaborator

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}>
Copy link
Collaborator

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">
Copy link
Collaborator

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
Copy link
Collaborator

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();
Copy link
Collaborator

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.

@KaiSpencer KaiSpencer requested a review from Tomdango September 27, 2021 21:24
@KaiSpencer
Copy link
Author

@Tomdango Sorry for the delay, think if addressed all comments :)

@KaiSpencer
Copy link
Author

Hey @Tomdango possible for some movement on this?

@kevinkuszyk
Copy link

@Tomdango ping.

What's the latest with this? Are we in good shape for merging?

/cc @KaiSpencer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants