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

chore(content-uploader): Migrate UploadInput #3594

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

greg-in-a-box
Copy link
Contributor

@greg-in-a-box greg-in-a-box commented Jul 31, 2024

  • Converted to typescript
  • Converted old .js to .js.flow
  • Converted to react-testing-library
  • Deleted snapshots
  • Added overriding react types to account for directory and webkidirectory on an input-

@greg-in-a-box greg-in-a-box requested a review from a team as a code owner July 31, 2024 19:28
@greg-in-a-box greg-in-a-box force-pushed the cu-uploadinput branch 5 times, most recently from f495a33 to 127c59f Compare July 31, 2024 20:32
Comment on lines +4 to +10
// https://stackoverflow.com/questions/72787050/typescript-upload-directory-property-directory-does-not-exist-on-type
// Extend the InputHTMLAttributes interface to include the directory attribute
declare module 'react' {
interface InputHTMLAttributes<T> extends React.HTMLAttributes<T> {
directory?: string;
webkitdirectory?: string;
}
}
Copy link
Contributor

@tjuanitas tjuanitas Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this exist in some global or common types location? other upload inputs would encounter the same error too right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only input we are doing this in, I didnt see a reason to let the rest of them know they have two more props

/* eslint-disable */ // Disabling linting for tabIndex on label, unknown why it's there
import * as React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we able to disable the specific error instead of the whole file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was the alt
// eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex,jsx-a11y/no-noninteractive-element-interactions <label className={inputLabelClass} onKeyDown={onKeyDown} tabIndex={0}>
thoughts on which one you think we should use?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my thinking is we want to be putting role="button" on the label

Copy link
Contributor Author

@greg-in-a-box greg-in-a-box Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gives the same lint error with role="button"
Screenshot 2024-08-01 at 12 37 31 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it narrowed it to a separate eslint error jsx-a11y/no-noninteractive-element-to-interactive-role which is more manageable

but we also disable the other two errors in a few of the elements already:

/* eslint-disable jsx-a11y/no-static-element-interactions */
/* eslint-disable jsx-a11y/no-noninteractive-tabindex */

}
}

export interface Props {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current pattern:

Suggested change
export interface Props {
export interface UploadInputProps {

Comment on lines 52 to 54
};
export default UploadInput;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit if you get to it:

Suggested change
};
export default UploadInput;
};
export default UploadInput;

Copy link
Contributor

@bfoxx1906 bfoxx1906 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify mergify bot merged commit c788333 into box:master Aug 14, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants