-
Notifications
You must be signed in to change notification settings - Fork 80
feat: add FileUploadCard and FileUploadStatus components #3109
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
base: feat/quick-order-by-file-feature-branch
Are you sure you want to change the base?
feat: add FileUploadCard and FileUploadStatus components #3109
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| // const UISearchInputField = dynamic<UISearchInputFieldProps & any>(() => | ||
| // /* webpackChunkName: "UISearchInputField" */ | ||
| // import('@faststore/ui').then((module) => module.SearchInputField) | ||
| // ) |
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.
Mudar a forma de importação
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.
Ajustado!
|
|
||
| &:focus-visible { | ||
| border-radius: var(--fs-border-radius-small); | ||
| outline: 2px solid var(--fs-color-focus-ring); |
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.
Devemos evitar usar px, usamos os tokens e quando não conseguimos, rem
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.
Ajustado!
ArthurTriis1
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.
O projeto da FS conta com storybook, pode adicionar uma versão simples para ajudar na revisão de novos componentes?
| height: 285px; | ||
| max-height: 285px; |
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.
Converter PX para rem, aqui e ao longo do código, exceto os stories
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.
Ajustado!
ArthurTriis1
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.
Aprovado, muito bom o resultado dos componentes e o storybook ajudou na correção. Deixei alguns comentários, favor corrigir antes do merge
lucasfp13
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.
Deixei algumas sugestões de ajustes, mas em relação à funcionalidade me parece ok.
|
|
||
| const isValidFileType = (file: File): boolean => { | ||
| const fileName = file.name.toLowerCase() | ||
| const validExtensions = ['.csv'] |
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.
As extensões válidas que o componente aceita, por padrão, são .csv, .xlsx e .xls, mas nesse array apenas a .csv está presente. lsso é intencional ou deveria ter as outras também?
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.
Devemos manter apenas .csv
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.
🎗️ Mudar o accept que é passado pro input para aceitar apenas csv
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.
Ajustado!
| aria-hidden={!isOpen} | ||
| {...otherProps} | ||
| > | ||
| <input |
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.
sugestão: Importar e usar aqui o átomo Input que temos no @faststore/ui
| <input | |
| <Input |
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.
Ajustado!
| Select file | ||
| </Button> | ||
|
|
||
| <button |
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.
Tem algum motivo específico de não usar o Button do @faststore/ui aqui também?
| <button | |
| <Button |
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.
Ajustado!
| ...otherProps | ||
| }: FileUploadStatusProps) => { | ||
| const formatFileSize = (bytes: number): string => { | ||
| return `${(bytes / 1024).toFixed(0)} KB` //ADJUST ONCE INTEGRATED |
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.
| return `${(bytes / 1024).toFixed(0)} KB` //ADJUST ONCE INTEGRATED | |
| return `${(bytes / 1024).toFixed(0)} KB` // TODO: ADJUST ONCE INTEGRATED |
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.
Ajustado!
| </div> | ||
|
|
||
| {onRemove && ( | ||
| <button |
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.
Tem algum motivo específico de não usar o Button do @faststore/ui aqui também?
| <button | |
| <Button |
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.
Ajustado!
| ref={ref} | ||
| buttonProps={buttonProps} | ||
| placeholder={placeholder} | ||
| showAttachmentButton={true} |
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.
| showAttachmentButton={true} | |
| showAttachmentButton |
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.
Ajustado!
| --fs-file-upload-card-padding: var(--fs-spacing-3); | ||
| --fs-file-upload-card-bkg-color: var(--fs-color-neutral-0); | ||
| --fs-file-upload-card-border-width: var(--fs-border-width); | ||
| --fs-file-upload-card-border-color: var(--fs-border-color-light); | ||
| --fs-file-upload-card-border-radius: var(--fs-border-radius-medium); | ||
|
|
||
| --fs-file-upload-card-dropzone-padding: | ||
| var(--fs-spacing-3) | ||
| var(--fs-spacing-4); | ||
| --fs-file-upload-card-dropzone-border-width: var(--fs-border-width-thick); | ||
| --fs-file-upload-card-dropzone-border-color: var(--fs-border-color-light); | ||
| --fs-file-upload-card-dropzone-border-radius: var(--fs-border-radius); | ||
|
|
||
| --fs-file-upload-card-icon-size: 3rem; | ||
| --fs-file-upload-card-badge-size: 1.5rem; | ||
| --fs-file-upload-card-badge-bg-color: #0366dd; | ||
| --fs-file-upload-card-shadow-color: #bfdbfe; | ||
|
|
||
| --fs-file-upload-card-title-color: var(--fs-color-text); | ||
| --fs-file-upload-card-link-color: var(--fs-color-link); |
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.
Apenas ajustando a indentação.
| --fs-file-upload-card-padding: var(--fs-spacing-3); | |
| --fs-file-upload-card-bkg-color: var(--fs-color-neutral-0); | |
| --fs-file-upload-card-border-width: var(--fs-border-width); | |
| --fs-file-upload-card-border-color: var(--fs-border-color-light); | |
| --fs-file-upload-card-border-radius: var(--fs-border-radius-medium); | |
| --fs-file-upload-card-dropzone-padding: | |
| var(--fs-spacing-3) | |
| var(--fs-spacing-4); | |
| --fs-file-upload-card-dropzone-border-width: var(--fs-border-width-thick); | |
| --fs-file-upload-card-dropzone-border-color: var(--fs-border-color-light); | |
| --fs-file-upload-card-dropzone-border-radius: var(--fs-border-radius); | |
| --fs-file-upload-card-icon-size: 3rem; | |
| --fs-file-upload-card-badge-size: 1.5rem; | |
| --fs-file-upload-card-badge-bg-color: #0366dd; | |
| --fs-file-upload-card-shadow-color: #bfdbfe; | |
| --fs-file-upload-card-title-color: var(--fs-color-text); | |
| --fs-file-upload-card-link-color: var(--fs-color-link); | |
| --fs-file-upload-card-padding: var(--fs-spacing-3); | |
| --fs-file-upload-card-bkg-color: var(--fs-color-neutral-0); | |
| --fs-file-upload-card-border-width: var(--fs-border-width); | |
| --fs-file-upload-card-border-color: var(--fs-border-color-light); | |
| --fs-file-upload-card-border-radius: var(--fs-border-radius-medium); | |
| --fs-file-upload-card-dropzone-padding: var(--fs-spacing-3) var(--fs-spacing-4); | |
| --fs-file-upload-card-dropzone-border-width: var(--fs-border-width-thick); | |
| --fs-file-upload-card-dropzone-border-color: var(--fs-border-color-light); | |
| --fs-file-upload-card-dropzone-border-radius: var(--fs-border-radius); | |
| --fs-file-upload-card-icon-size: 3rem; | |
| --fs-file-upload-card-badge-size: 1.5rem; | |
| --fs-file-upload-card-badge-bg-color: #0366dd; | |
| --fs-file-upload-card-shadow-color: #bfdbfe; | |
| --fs-file-upload-card-title-color: var(--fs-color-text); | |
| --fs-file-upload-card-link-color: var(--fs-color-link); |
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.
Ajustado!
| --fs-file-upload-status-padding: var(--fs-spacing-0); | ||
| --fs-file-upload-status-gap: var(--fs-spacing-3); | ||
| --fs-file-upload-status-bkg-color: var(--fs-color-neutral-0); | ||
|
|
||
| --fs-file-upload-status-file-info-padding: var(--fs-spacing-3); | ||
| --fs-file-upload-status-file-info-border-color: var(--fs-border-color-light); | ||
| --fs-file-upload-status-file-info-border-radius: 1.25rem; | ||
|
|
||
| --fs-file-upload-status-icon-size: var(--fs-spacing-6); | ||
| --fs-file-upload-status-icon-bg-completed: #08a822; | ||
| --fs-file-upload-status-icon-bg-uploading: #08a822; | ||
| --fs-file-upload-status-icon-bg-error: #d31a15; | ||
|
|
||
| --fs-file-upload-status-filename-color: var(--fs-color-text); | ||
| --fs-file-upload-status-text-color: var(--fs-color-text-light); | ||
|
|
||
| --fs-file-upload-status-button-width: 100%; | ||
| --fs-file-upload-status-button-height: var(--fs-spacing-7); |
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.
| --fs-file-upload-status-padding: var(--fs-spacing-0); | |
| --fs-file-upload-status-gap: var(--fs-spacing-3); | |
| --fs-file-upload-status-bkg-color: var(--fs-color-neutral-0); | |
| --fs-file-upload-status-file-info-padding: var(--fs-spacing-3); | |
| --fs-file-upload-status-file-info-border-color: var(--fs-border-color-light); | |
| --fs-file-upload-status-file-info-border-radius: 1.25rem; | |
| --fs-file-upload-status-icon-size: var(--fs-spacing-6); | |
| --fs-file-upload-status-icon-bg-completed: #08a822; | |
| --fs-file-upload-status-icon-bg-uploading: #08a822; | |
| --fs-file-upload-status-icon-bg-error: #d31a15; | |
| --fs-file-upload-status-filename-color: var(--fs-color-text); | |
| --fs-file-upload-status-text-color: var(--fs-color-text-light); | |
| --fs-file-upload-status-button-width: 100%; | |
| --fs-file-upload-status-button-height: var(--fs-spacing-7); | |
| --fs-file-upload-status-padding: var(--fs-spacing-0); | |
| --fs-file-upload-status-gap: var(--fs-spacing-3); | |
| --fs-file-upload-status-bkg-color: var(--fs-color-neutral-0); | |
| --fs-file-upload-status-file-info-padding: var(--fs-spacing-3); | |
| --fs-file-upload-status-file-info-border-color: var(--fs-border-color-light); | |
| --fs-file-upload-status-file-info-border-radius: 1.25rem; | |
| --fs-file-upload-status-icon-size: var(--fs-spacing-6); | |
| --fs-file-upload-status-icon-bg-completed: #08a822; | |
| --fs-file-upload-status-icon-bg-uploading: #08a822; | |
| --fs-file-upload-status-icon-bg-error: #d31a15; | |
| --fs-file-upload-status-filename-color: var(--fs-color-text); | |
| --fs-file-upload-status-text-color: var(--fs-color-text-light); | |
| --fs-file-upload-status-button-width: 100%; | |
| --fs-file-upload-status-button-height: var(--fs-spacing-7); |
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.
Ajustado!
Estranho que o proprio lint quando commita remove os espaços! Mas nao commitei ele ajustado.
| items?: Value | ||
| postalCode?: Value | ||
| } | ||
|
|
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.
Oie!
Acredito que esteja faltando rebase nas branchs dessa feature, porque está misturado com mudanças que já estão na main da faststore. Essa do jwt, por exemplo.
- Introduced `FileUploadCard` for file selection and upload functionality, including drag-and-drop support. - Added `FileUploadStatus` to display upload progress and error handling. - Updated index files to export new components and types. - Enhanced `SearchInputField` to include an attachment button that triggers the file upload card. - Added styles for both new components to ensure proper UI integration.
- Replaced native input elements with custom Input and Button components for consistency. - Simplified accepted file types in FileUploadCard to only '.csv'. - Added TODO comment for file size formatting in FileUploadStatus. - Adjusted SCSS variable formatting for improved readability.
📝 WalkthroughWalkthroughThis PR introduces FileUploadCard and FileUploadStatus React components with drag-and-drop file selection, state management for upload workflows, and integration into SearchInput. Includes comprehensive styling, public API exports, and Storybook demonstrations. Changes
Sequence DiagramsequenceDiagram
actor User
participant Card as FileUploadCard
participant Status as FileUploadStatus
participant Handler as Parent Component
User->>Card: Click "Select file" / Drag-drop
Card->>Card: Validate file (extension/type)
alt File valid
Card->>Card: Set selectedFile & uploadState='uploading'
Card->>Status: Render with uploading state
User->>Status: Monitor upload progress
Card->>Card: Simulate 2s upload delay
Card->>Card: uploadState='completed'
Status->>Status: Display success icon
User->>Status: Click "Search" or "Remove"
Status->>Handler: onSearch() / onRemove() callback
else File invalid
Card->>Card: Set uploadState='error'
Card->>Status: Render with error state
User->>Status: Click "Download template" or "Select file"
Status->>Handler: onDownloadTemplate() / onSelectFile() callback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/components/src/molecules/FileUploadCard/FileUploadCard.tsx`:
- Around line 108-111: The simulated upload setTimeout in FileUploadCard
currently calls setUploadState('completed') without cleanup, risking state
updates after unmount; modify the component to store the timer id (e.g., in a
ref) and set the timeout inside a useEffect that returns a cleanup which clears
the timeout, or cancel the timer in an existing unmount effect so setUploadState
is never called after the component unmounts.
In `@packages/components/src/molecules/FileUploadStatus/FileUploadStatus.tsx`:
- Around line 202-233: The action buttons in FileUploadStatus (the Search button
with data-fs-file-upload-status-search-button, and the Download template and
Select file buttons with data-fs-file-upload-status-download-button and
data-fs-file-upload-status-select-button) are missing an explicit type and will
default to submit when rendered inside a form; update each Button element
rendered under state === 'completed' (onSearch) and state === 'error'
(onDownloadTemplate, onSelectFile) to include type="button" so they do not
trigger form submission.
In `@packages/storybook/stories/fileupload.stories.tsx`:
- Around line 133-143: The handleDownloadTemplate function currently triggers a
download by clicking a detached anchor which can be ignored by some browsers
(e.g., Firefox); modify handleDownloadTemplate to append the created <a> element
to document.body before calling a.click(), then remove the anchor after the
click and revoke the object URL (keep creating the Blob, setting
a.href/a.download as-is) to improve cross-browser reliability.
In `@packages/ui/src/components/molecules/SearchInputField/styles.scss`:
- Around line 109-122: There is a duplicate CSS rule for the selector
[data-fs-search-input-field-input]; remove the second block (the one that sets
padding-right) and consolidate any unique properties (background-color,
transition, height, and the media query) into the first definition so the rule
uses padding-inline-end for RTL support; ensure the merged rule retains the
media query (`@include` media("<notebook") { border: 0; }) and the
transition/background-color/height settings from the duplicate block.
- Around line 69-78: Remove the orphaned CSS properties block (the standalone
rules: right, display, gap, align-items and the media query) that are present
without a selector; these are duplicate/leftover and will break SCSS. Locate the
stray block near the end of the file and delete it, keeping the existing valid
styling defined under the [data-fs-search-input-field-actions] selector (which
already defines the same rules), and ensure only the selector-scoped styles
remain.
🧹 Nitpick comments (8)
packages/core/src/components/search/SearchInput/SearchInput.tsx (2)
115-120: Incomplete file upload handling.The
handleFileSelectfunction sets state but has a TODO indicating the actual file upload logic is not implemented. The commented-outsetFileUploadVisible(false)suggests uncertainty about the intended behavior.Consider completing the implementation or tracking this as a follow-up task.
Would you like me to open an issue to track the implementation of the actual file upload logic?
212-219: RedundantisOpenprop condition.The
isOpenprop evaluates toisUploadOpen || hasFile || fileUploadVisible, but since this block only renders whenfileUploadVisibleis true,isOpenwill always betrue. TheisUploadOpenandhasFileconditions have no effect here.Simplify the condition
{fileUploadVisible && ( <FileUploadCard - isOpen={isUploadOpen || hasFile || fileUploadVisible} + isOpen={fileUploadVisible} onDismiss={() => setFileUploadVisible(false)} onFileSelect={handleFileSelect} onDownloadTemplate={handleDownloadTemplate} /> )}Or consider removing
isUploadOpenandhasFilestates if they're not needed elsewhere.packages/ui/src/components/molecules/FileUploadStatus/styles.scss (2)
13-15: Hardcoded colors reduce theming flexibility.The icon background colors use hardcoded hex values (
#08a822,#d31a15) instead of design tokens. This makes it harder to maintain consistent theming across the application.Consider using design tokens
- --fs-file-upload-status-icon-bg-completed: `#08a822`; - --fs-file-upload-status-icon-bg-uploading: `#08a822`; - --fs-file-upload-status-icon-bg-error: `#d31a15`; + --fs-file-upload-status-icon-bg-completed: var(--fs-color-success, `#08a822`); + --fs-file-upload-status-icon-bg-uploading: var(--fs-color-success, `#08a822`); + --fs-file-upload-status-icon-bg-error: var(--fs-color-danger, `#d31a15`);This applies to other hardcoded colors in this file as well (lines 48-49, 198-200, 220-226).
29-29: Remove commented-out code.The commented
height: 188px;should either be removed or documented if it serves as a reference.packages/ui/src/components/molecules/FileUploadCard/styles.scss (2)
49-51: Useremunits for fixed height.The fixed height of
285pxshould be converted toremunits for consistency with the design system. Based on past review comments, pixel units should be avoided except in stories.Convert to rem
&[data-fs-file-upload-card-open="true"] { - height: 285px; - max-height: 285px; + height: 17.8125rem; + max-height: 17.8125rem; pointer-events: auto;
17-18: Hardcoded colors reduce theming flexibility.The badge and shadow colors use hardcoded hex values. Consider using design tokens for better maintainability and theming support.
Consider using design tokens
- --fs-file-upload-card-badge-bg-color: `#0366dd`; - --fs-file-upload-card-shadow-color: `#bfdbfe`; + --fs-file-upload-card-badge-bg-color: var(--fs-color-primary, `#0366dd`); + --fs-file-upload-card-shadow-color: var(--fs-color-primary-bkg-light, `#bfdbfe`);This pattern applies to other hardcoded colors in this file (lines 161, 189-196).
packages/components/src/molecules/FileUploadCard/FileUploadCard.tsx (1)
92-117: Duplicate file processing logic.
handleFileChangeandhandleDropcontain nearly identical file processing logic (validation, state updates, simulated upload). Extract this into a shared helper function.Extract shared file processing logic
+ const processFile = (file: File, files: File[]) => { + setSelectedFile(file) + + if (!isValidFileType(file)) { + setUploadState('error') + setErrorType('unsupported') + return + } + + setUploadState('uploading') + setErrorType(undefined) + + // Simulate upload process + setTimeout(() => { + setUploadState('completed') + }, 2000) + + onFileSelect?.(files) + } + const handleFileChange = (e: ChangeEvent<HTMLInputElement>) => { const files = Array.from(e.target.files || []) if (files.length > 0) { - const file = files[0] - setSelectedFile(file) - - // Validate file type - if (!isValidFileType(file)) { - setUploadState('error') - setErrorType('unsupported') - return - } - - setUploadState('uploading') - setErrorType(undefined) - - // Simulate upload process - setTimeout(() => { - setUploadState('completed') - }, 2000) - - if (onFileSelect) { - onFileSelect(files) - } + processFile(files[0], files) } }Apply similar refactoring to
handleDrop.Also applies to: 130-160
packages/storybook/stories/fileupload.stories.tsx (1)
1-7: Use the public type export instead of a deep dist import.Deep imports can break with package refactors or alternate build outputs. Prefer the root export if available; otherwise consider exporting
FileUploadErrorTypefrom the package index.🔧 Proposed change
-import type { FileUploadErrorType } from '@faststore/components/dist/esm/molecules/FileUploadStatus' +import type { FileUploadErrorType } from '@faststore/components'
| // Simulate upload process | ||
| setTimeout(() => { | ||
| setUploadState('completed') | ||
| }, 2000) |
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.
Potential state update on unmounted component.
The setTimeout for simulating upload completion doesn't have cleanup logic. If the component unmounts before the 2-second timeout completes, React will warn about updating state on an unmounted component.
Add cleanup with useRef or useEffect
+ const uploadTimeoutRef = useRef<NodeJS.Timeout | null>(null)
+
+ useEffect(() => {
+ return () => {
+ if (uploadTimeoutRef.current) {
+ clearTimeout(uploadTimeoutRef.current)
+ }
+ }
+ }, [])
+
// In processFile or handleFileChange:
- setTimeout(() => {
+ uploadTimeoutRef.current = setTimeout(() => {
setUploadState('completed')
}, 2000)🤖 Prompt for AI Agents
In `@packages/components/src/molecules/FileUploadCard/FileUploadCard.tsx` around
lines 108 - 111, The simulated upload setTimeout in FileUploadCard currently
calls setUploadState('completed') without cleanup, risking state updates after
unmount; modify the component to store the timer id (e.g., in a ref) and set the
timeout inside a useEffect that returns a cleanup which clears the timeout, or
cancel the timer in an existing unmount effect so setUploadState is never called
after the component unmounts.
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.
We can ignore this comment since the setTimeout is only for testing purposes
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.
@lariciamota, understood! Since this is a simulation for testing purposes, the timeout cleanup isn't critical here.
If you'd like, you could add a brief comment noting this is intentional test/demo behavior to prevent future reviewers from flagging it again, but that's entirely optional.
✏️ Learnings added
Learnt from: lariciamota
Repo: vtex/faststore PR: 3109
File: packages/components/src/molecules/FileUploadCard/FileUploadCard.tsx:108-111
Timestamp: 2026-01-20T12:59:11.210Z
Learning: In `packages/components/src/molecules/FileUploadCard/FileUploadCard.tsx`, the setTimeout that simulates upload completion (setting uploadState to 'completed' after 2 seconds) is intentional for testing/demo purposes and does not require cleanup.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| {state === 'completed' && onSearch && ( | ||
| <Button | ||
| variant="primary" | ||
| size="regular" | ||
| onClick={onSearch} | ||
| data-fs-file-upload-status-search-button | ||
| > | ||
| Search | ||
| </Button> | ||
| )} | ||
|
|
||
| {state === 'error' && ( | ||
| <div data-fs-file-upload-status-error-actions> | ||
| {onDownloadTemplate && ( | ||
| <Button | ||
| variant="secondary" | ||
| size="regular" | ||
| onClick={onDownloadTemplate} | ||
| data-fs-file-upload-status-download-button | ||
| > | ||
| Download template | ||
| </Button> | ||
| )} | ||
| {onSelectFile && ( | ||
| <Button | ||
| variant="primary" | ||
| size="regular" | ||
| onClick={onSelectFile} | ||
| data-fs-file-upload-status-select-button | ||
| > | ||
| Select file | ||
| </Button> |
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.
Set non-submit action buttons to type="button".
Without an explicit type, these buttons default to submit inside a form, which can trigger unintended form submissions when this component is rendered within a form context.
Proposed fix
{state === 'completed' && onSearch && (
<Button
+ type="button"
variant="primary"
size="regular"
onClick={onSearch}
data-fs-file-upload-status-search-button
>
Search
</Button>
)}
{state === 'error' && (
<div data-fs-file-upload-status-error-actions>
{onDownloadTemplate && (
<Button
+ type="button"
variant="secondary"
size="regular"
onClick={onDownloadTemplate}
data-fs-file-upload-status-download-button
>
Download template
</Button>
)}
{onSelectFile && (
<Button
+ type="button"
variant="primary"
size="regular"
onClick={onSelectFile}
data-fs-file-upload-status-select-button
>
Select file
</Button>
)}
</div>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {state === 'completed' && onSearch && ( | |
| <Button | |
| variant="primary" | |
| size="regular" | |
| onClick={onSearch} | |
| data-fs-file-upload-status-search-button | |
| > | |
| Search | |
| </Button> | |
| )} | |
| {state === 'error' && ( | |
| <div data-fs-file-upload-status-error-actions> | |
| {onDownloadTemplate && ( | |
| <Button | |
| variant="secondary" | |
| size="regular" | |
| onClick={onDownloadTemplate} | |
| data-fs-file-upload-status-download-button | |
| > | |
| Download template | |
| </Button> | |
| )} | |
| {onSelectFile && ( | |
| <Button | |
| variant="primary" | |
| size="regular" | |
| onClick={onSelectFile} | |
| data-fs-file-upload-status-select-button | |
| > | |
| Select file | |
| </Button> | |
| {state === 'completed' && onSearch && ( | |
| <Button | |
| type="button" | |
| variant="primary" | |
| size="regular" | |
| onClick={onSearch} | |
| data-fs-file-upload-status-search-button | |
| > | |
| Search | |
| </Button> | |
| )} | |
| {state === 'error' && ( | |
| <div data-fs-file-upload-status-error-actions> | |
| {onDownloadTemplate && ( | |
| <Button | |
| type="button" | |
| variant="secondary" | |
| size="regular" | |
| onClick={onDownloadTemplate} | |
| data-fs-file-upload-status-download-button | |
| > | |
| Download template | |
| </Button> | |
| )} | |
| {onSelectFile && ( | |
| <Button | |
| type="button" | |
| variant="primary" | |
| size="regular" | |
| onClick={onSelectFile} | |
| data-fs-file-upload-status-select-button | |
| > | |
| Select file | |
| </Button> |
🤖 Prompt for AI Agents
In `@packages/components/src/molecules/FileUploadStatus/FileUploadStatus.tsx`
around lines 202 - 233, The action buttons in FileUploadStatus (the Search
button with data-fs-file-upload-status-search-button, and the Download template
and Select file buttons with data-fs-file-upload-status-download-button and
data-fs-file-upload-status-select-button) are missing an explicit type and will
default to submit when rendered inside a form; update each Button element
rendered under state === 'completed' (onSearch) and state === 'error'
(onDownloadTemplate, onSelectFile) to include type="button" so they do not
trigger form submission.
| const handleDownloadTemplate = () => { | ||
| console.log('Download template clicked') | ||
| // Create and download template file | ||
| const csvContent = 'Product ID,Quantity,Price\n001,10,99.99\n002,5,49.99' | ||
| const blob = new Blob([csvContent], { type: 'text/csv' }) | ||
| const url = window.URL.createObjectURL(blob) | ||
| const a = document.createElement('a') | ||
| a.href = url | ||
| a.download = 'template.csv' | ||
| a.click() | ||
| window.URL.revokeObjectURL(url) |
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.
Make the template download more browser‑compatible.
Some browsers (notably Firefox) can ignore clicks on detached anchors. Appending/removing the node improves reliability.
🔧 Proposed change
const a = document.createElement('a')
a.href = url
a.download = 'template.csv'
- a.click()
- window.URL.revokeObjectURL(url)
+ document.body.appendChild(a)
+ a.click()
+ a.remove()
+ window.URL.revokeObjectURL(url)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleDownloadTemplate = () => { | |
| console.log('Download template clicked') | |
| // Create and download template file | |
| const csvContent = 'Product ID,Quantity,Price\n001,10,99.99\n002,5,49.99' | |
| const blob = new Blob([csvContent], { type: 'text/csv' }) | |
| const url = window.URL.createObjectURL(blob) | |
| const a = document.createElement('a') | |
| a.href = url | |
| a.download = 'template.csv' | |
| a.click() | |
| window.URL.revokeObjectURL(url) | |
| const handleDownloadTemplate = () => { | |
| console.log('Download template clicked') | |
| // Create and download template file | |
| const csvContent = 'Product ID,Quantity,Price\n001,10,99.99\n002,5,49.99' | |
| const blob = new Blob([csvContent], { type: 'text/csv' }) | |
| const url = window.URL.createObjectURL(blob) | |
| const a = document.createElement('a') | |
| a.href = url | |
| a.download = 'template.csv' | |
| document.body.appendChild(a) | |
| a.click() | |
| a.remove() | |
| window.URL.revokeObjectURL(url) | |
| } |
🤖 Prompt for AI Agents
In `@packages/storybook/stories/fileupload.stories.tsx` around lines 133 - 143,
The handleDownloadTemplate function currently triggers a download by clicking a
detached anchor which can be ignored by some browsers (e.g., Firefox); modify
handleDownloadTemplate to append the created <a> element to document.body before
calling a.click(), then remove the anchor after the click and revoke the object
URL (keep creating the Blob, setting a.href/a.download as-is) to improve
cross-browser reliability.
| right: 0.125rem; | ||
| display: flex; | ||
| gap: 0; | ||
| align-items: center; | ||
|
|
||
| @include media(">=notebook") { | ||
| position: absolute; | ||
| right: 0; | ||
| } | ||
| } |
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.
Orphaned CSS properties will cause issues.
Lines 69-78 contain CSS properties (right, display, gap, align-items, media query) without a proper selector. This appears to be a merge conflict artifact or copy-paste error. These properties will either cause SCSS compilation errors or be ignored.
Remove orphaned properties
These lines should be removed as the same styling is already defined in [data-fs-search-input-field-actions] block (lines 48-59):
[data-fs-search-input-field-input] {
height: auto;
padding-inline-end: var(--fs-search-input-field-input-padding-right);
background-color: var(--fs-search-input-field-input-bkg-color);
transition: box-shadow var(--fs-search-input-field-transition-timing) var(--fs-search-input-field-transition-timing), border var(--fs-search-input-field-transition-timing) var(--fs-search-input-field-transition-function);
`@include` media("<notebook") { border: 0; }
}
- right: 0.125rem;
- display: flex;
- gap: 0;
- align-items: center;
-
- `@include` media(">=notebook") {
- position: absolute;
- right: 0;
- }
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| right: 0.125rem; | |
| display: flex; | |
| gap: 0; | |
| align-items: center; | |
| @include media(">=notebook") { | |
| position: absolute; | |
| right: 0; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/ui/src/components/molecules/SearchInputField/styles.scss` around
lines 69 - 78, Remove the orphaned CSS properties block (the standalone rules:
right, display, gap, align-items and the media query) that are present without a
selector; these are duplicate/leftover and will break SCSS. Locate the stray
block near the end of the file and delete it, keeping the existing valid styling
defined under the [data-fs-search-input-field-actions] selector (which already
defines the same rules), and ensure only the selector-scoped styles remain.
| [data-fs-search-input-field-input] { | ||
| height: auto; | ||
| padding-right: var(--fs-spacing-7); | ||
| background-color: var(--fs-search-input-field-input-bkg-color); | ||
| transition: | ||
| box-shadow var(--fs-search-input-field-transition-timing) | ||
| var(--fs-search-input-field-transition-timing), | ||
| border var(--fs-search-input-field-transition-timing) | ||
| var(--fs-search-input-field-transition-function); | ||
|
|
||
| @include media("<notebook") { | ||
| border: 0; | ||
| } | ||
| } |
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.
Duplicate selector definition.
[data-fs-search-input-field-input] is defined twice (lines 61-68 and 109-122). The second definition uses padding-right while the first uses padding-inline-end (which is better for RTL support). These should be consolidated.
Consolidate duplicate definitions
Remove this duplicate block and keep the first definition at lines 61-68 which uses the more appropriate padding-inline-end:
- [data-fs-search-input-field-input] {
- height: auto;
- padding-right: var(--fs-spacing-7);
- background-color: var(--fs-search-input-field-input-bkg-color);
- transition:
- box-shadow var(--fs-search-input-field-transition-timing)
- var(--fs-search-input-field-transition-timing),
- border var(--fs-search-input-field-transition-timing)
- var(--fs-search-input-field-transition-function);
-
- `@include` media("<notebook") {
- border: 0;
- }
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [data-fs-search-input-field-input] { | |
| height: auto; | |
| padding-right: var(--fs-spacing-7); | |
| background-color: var(--fs-search-input-field-input-bkg-color); | |
| transition: | |
| box-shadow var(--fs-search-input-field-transition-timing) | |
| var(--fs-search-input-field-transition-timing), | |
| border var(--fs-search-input-field-transition-timing) | |
| var(--fs-search-input-field-transition-function); | |
| @include media("<notebook") { | |
| border: 0; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/ui/src/components/molecules/SearchInputField/styles.scss` around
lines 109 - 122, There is a duplicate CSS rule for the selector
[data-fs-search-input-field-input]; remove the second block (the one that sets
padding-right) and consolidate any unique properties (background-color,
transition, height, and the media query) into the first definition so the rule
uses padding-inline-end for RTL support; ensure the merged rule retains the
media query (`@include` media("<notebook") { border: 0; }) and the
transition/background-color/height settings from the duplicate block.
| const [searchQuery, setSearchQuery] = useState('') | ||
|
|
||
| const handleDownloadTemplate = () => { | ||
| console.log('Download template clicked') |
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.
Podemos já apagar esse console.log, os outros pode deixar já que as outras funções estão sem ação ainda
| console.log('Download template clicked') |
Tasks https://vtex-dev.atlassian.net/jira/software/projects/BE/boards/2868?selectedIssue=BE-69 BE-70
Here's the completed PR description in English:
FileUploadCardfor file selection and upload functionality, including drag-and-drop support.FileUploadStatusto display upload progress and error handling.SearchInputFieldto include an attachment button that triggers the file upload card.What's the purpose of this pull request?
This PR introduces a new file upload feature that allows users to perform bulk searches by uploading CSV files. The feature provides a modern, user-friendly interface with drag-and-drop support, real-time upload status tracking, and comprehensive error handling to guide users through the file upload process.
How it works?
FileUploadCard Component
.csvextension)FileUploadStatus Component
Displays the current status of the uploaded file with three distinct states:
Uploading State
Completed State
Error State
Error Types
The component handles six different error scenarios:
Integration
SearchInputFieldcomponentFileUploadCardvisibilityKey Features
How to test it?
Basic File Upload Flow
Drag and Drop
Error Handling - Unsupported File Type
File Removal
Starters Deploy Preview
References
Checklist
You may erase this after checking them all 😉
PR Title and Commit Messages
feat,fix,chore,docs,style,refactor,ciandtestPR Description
breaking change,bug,contributing,performance,documentation..Dependencies
pnpm-lock.yamlfile when there were changes to the packagesDocumentation
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.