-
Notifications
You must be signed in to change notification settings - Fork 17
or-1730: introduce file input o3 form #2313
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
Conversation
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.
Pull request overview
This PR introduces a new file input component to the o3-form library, allowing users to upload files with visual feedback and delete functionality.
Key changes:
- New FileInput component with upload/delete capabilities and uploading state indication
- Enhanced focus styles to support
:focus-withinpseudo-class for better accessibility - Updated story decorators across form components to use dynamic brand selection from globals
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| libraries/o3-figma-sb-links/src/sb-links.json | Added Storybook links for file input stories |
| libraries/o3-figma-sb-links/src/links.json | Added navigation links for file input component stories |
| libraries/o3-figma-sb-links/src/figma-links.json | Added Figma design link placeholders for file input |
| components/o3-foundation/src/css/components/focus.css | Added :focus-within pseudo-class support for focus rings |
| components/o3-form/stories/*.stories.tsx | Updated decorators to use dynamic brand selection |
| components/o3-form/stories/fileInput.test.stories.tsx | Added test stories for file input JavaScript functionality |
| components/o3-form/stories/fileInput.stories.tsx | Added main file input stories |
| components/o3-form/stories/FileInputJavaScript.tsx | Created helper component for JavaScript-based file input demos |
| components/o3-form/src/types/index.ts | Added FileInputProps type definition |
| components/o3-form/src/tsx/index.ts | Exported FileInput component |
| components/o3-form/src/tsx/FileUploadController.ts | Implemented file upload controller logic |
| components/o3-form/src/tsx/FileInput.tsx | Created FileInput React component |
| components/o3-form/src/css/components/file-input.css | Added file input styling |
| components/o3-form/main.css | Imported file-input.css |
| components/o3-form/README.md | Added documentation for file input component |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
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.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
File upload content should not flex via column, so we should remove the o3-form class
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.
Here is a few of comments from me. As I am relatively new to the codebase, I would appreciate that @j-mes will provide a better review for this new component 😄
| } | ||
|
|
||
| :root { | ||
| --o3-file-input-icon-delete: url("data:image/svg+xml, %3Csvg xmlns='http://www.w3.org/2000/svg' width='28' height='28' viewBox='0 0 28 28' fill='none'%3E%3Cpath d='M12.3337 17.3333C12.1496 17.3333 12.0003 17.1841 12.0003 17L12.0003 12.3333C12.0003 12.1492 12.1496 12 12.3337 12H13.0003C13.1844 12 13.3337 12.1492 13.3337 12.3333L13.3337 17C13.3337 17.1841 13.1844 17.3333 13.0003 17.3333H12.3337Z' fill='black'/%3E%3Cpath d='M14.667 17C14.667 17.1841 14.8162 17.3333 15.0003 17.3333H15.667C15.8511 17.3333 16.0003 17.1841 16.0003 17V12.3333C16.0003 12.1492 15.8511 12 15.667 12H15.0003C14.8162 12 14.667 12.1492 14.667 12.3333V17Z' fill='black'/%3E%3Cpath fill-rule='evenodd' clip-rule='evenodd' d='M13.0003 8C12.8162 8 12.667 8.14924 12.667 8.33333V9.33333H9.00033C8.81623 9.33333 8.66699 9.48257 8.66699 9.66667V10.3333C8.66699 10.5174 8.81623 10.6667 9.00033 10.6667H9.33366V19.3333C9.33366 19.7015 9.63214 20 10.0003 20H18.0003C18.3685 20 18.667 19.7015 18.667 19.3333V10.6667H19.0003C19.1844 10.6667 19.3337 10.5174 19.3337 10.3333V9.66667C19.3337 9.48257 19.1844 9.33333 19.0003 9.33333H15.3337V8.33333C15.3337 8.14924 15.1844 8 15.0003 8H13.0003ZM10.667 10.6667H17.3337V18.6667H10.667V10.6667Z' fill='black'/%3E%3C/svg%3E") |
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.
suggestion: We should reference this icon from o3 foundations.
| font-family: var(--o3-type-body-base-font-family); | ||
| font-size: var(--o3-type-body-base-font-size); | ||
| font-weight: var(--o3-type-body-base-font-weight); | ||
| line-height: var(--o3-type-body-base-line-height); |
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.
thought: I would like for us at some point to make use of the typography utility classes within our components to reduce repeating rules.
| Story => ( | ||
| <div data-o3-brand="core"> | ||
| (Story, {args, globals}) => ( | ||
| <div data-o3-brand={globals.selectedBrand || 'whitelabel'}> |
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 like this change very much
| Story => ( | ||
| <div data-o3-brand="core"> | ||
| (Story, {args, globals}) => ( | ||
| <div data-o3-brand={globals.selectedBrand || 'whitelabel'} data-o3-theme={globals.selectedTheme || 'standard'}> |
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.
is standard defined as core by default?
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.
is
standarddefined ascoreby default?
Nope, standard is a theme here. core is a brand.
j-mes
left a comment
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.
Just some minor comments and I'm curious about @joelcarr's approach with typography utility classes too!
Describe your changes
Additional context
Checklist before requesting a review
percylabel for o-[COMPONENT] orchromaticlabel for o3-[COMPONENT] on my PR before merging and after review. Find more details in CONTRIBUTING.md