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.

Sign up for GitHub

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

Jump to bottom

feat: add file uploader as a fragment component #302

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

feat: add file uploader as a fragment component #302

wants to merge 9 commits into from

Conversation

maxxyouu
Copy link
Contributor

@maxxyouu maxxyouu commented Mar 7, 2024 •
edited
Loading

for #184

Max You added 3 commits March 6, 2024 22:16
initial commit
03734c2 
Signed-off-by: Max You <[email protected]>
initial commit
7d6c4e9 
Signed-off-by: Max You <[email protected]>
slot
4c087b0 
Signed-off-by: Max You <[email protected]>
@netlify Netlify
Copy link

netlify bot commented Mar 7, 2024 •
edited
Loading

👷 Deploy request for carbon-components-builder pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit eaebae1

Max You added 2 commits March 13, 2024 15:30
fix build err
c2af9cd 
Signed-off-by: Max You <[email protected]>
fixed export and checkbvox
945f0c7 
Signed-off-by: Max You <[email protected]>
@maxxyouu
Copy link
Contributor Author

Verified that the component features and export works.

Max You added 3 commits April 19, 2024 12:23
@Akshat55 Akshat55 changed the title feat: file uploader feat: add file uploader as a fragment component May 22, 2024
progress changes
eaebae1 
Signed-off-by: Max You <[email protected]>
zvonimirfras
zvonimirfras requested changes Jul 16, 2024
View reviewed changes
Copy link
Member

@zvonimirfras zvonimirfras left a comment

Choose a reason for hiding this comment

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

messy

player/react/src/lib/components/ui-file-uploader.tsx
Comment on lines +10 to +21
acceptedFileFormat: string;
buttonKind: string;
buttonLabel: string;
labelTitle: string;
filenameStatus: string;
dragAndDroplabelText: string;
labelDescription: string;
iconDescription: string;
size: string;
multiple: boolean;
disabled: boolean;
dragAndDrop: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

some of these (maybe all?) should be optional

player/react/src/lib/components/ui-file-uploader.tsx
...commonSlots,
...slotsDisabled,
multiple: 'boolean',
isMultiple: (state: FileUploaderState) => ({
Copy link
Member

Choose a reason for hiding this comment

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

same as for time picker

player/react/src/lib/components/ui-file-uploader.tsx
onChange={(event: any) => {
sendSignal(state.id, 'valueChange', [event.value], { ...state, value: event.value });
}}
accept={state.acceptedFileFormat.split(',')}
Copy link
Member

Choose a reason for hiding this comment

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

looks like acceptedFileFormat should be acceptedFileFormats and be a list of strings

player/react/src/lib/components/ui-file-uploader.tsx
buttonLabel: 'string',
labelTitle: 'string',
filenameStatus: 'string',
dragAndDroplabelText: 'string',
Copy link
Member

Choose a reason for hiding this comment

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

something's off about this

sdk/react/src/lib/fragment-components/a-file-uploader.tsx
Comment on lines +6 to +7
import { Checkbox, Dropdown, TextInput, FileUploaderDropContainer, FileUploader } from '@carbon/react';
import { angularClassNamesFromComponentObj, nameStringToVariableString, reactClassNamesFromComponentObj } from '../helpers/tools';
Copy link
Member

Choose a reason for hiding this comment

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

looks like these would be better split into multiple lines to follow the style in other files

sdk/react/src/lib/fragment-components/a-file-uploader.tsx
...selectedComponent,
iconDescription: event.currentTarget.value
})} />
</>
Copy link
Member

Choose a reason for hiding this comment

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

Something's off with the indentation here

sdk/react/src/lib/fragment-components/a-file-uploader.tsx
Comment on lines +192 to +209
componentObj.dragAndDrop ? <FileUploaderDropContainer
className={cx(preventCheckEventStyle, componentObj.cssClasses?.map((cc: any) => cc.id).join(' '))}
accept={acceptedFileFormat}
multiple={componentObj.multiple}
disabled={componentObj.disabled}
labelText={componentObj.dragAndDroplabelText}
tabIndex={0} /> : <FileUploader
className={cx(preventCheckEventStyle, componentObj.cssClasses?.map((cc: any) => cc.id).join(' '))}
accept={acceptedFileFormat}
buttonKind={componentObj.buttonKind}
buttonLabel={componentObj.buttonLabel}
filenameStatus={componentObj.filenameStatus}
iconDescription={componentObj.iconDescription}
labelDescription={componentObj.labelDescription}
labelTitle={componentObj.labelTitle}
multiple={componentObj.multiple}
disabled={componentObj.disabled}
size={componentObj.size} />
Copy link
Member

Choose a reason for hiding this comment

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

this should be more readable. Follow the style from the rest of the repo (as suggested above)

sdk/react/src/lib/fragment-components/a-file-uploader.tsx
Comment on lines +266 to +268
[disabled]="${nameStringToVariableString(json.codeContext?.name)}Disabled"
[size]="${nameStringToVariableString(json.codeContext?.name)}Size"
[disabled]="${nameStringToVariableString(json.codeContext?.name)}Disabled"
Copy link
Member

Choose a reason for hiding this comment

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

disabled twice??

sdk/react/src/lib/fragment-components/a-file-uploader.tsx
${json.dragAndDrop ? `[dropText]="${nameStringToVariableString(json.codeContext?.name)}DropText"` : ''}
${json.dragAndDrop ? '' : `[buttonText]="${nameStringToVariableString(json.codeContext?.name)}ButtonText"`}
${json.dragAndDrop ? '' : `[buttonType]="${nameStringToVariableString(json.codeContext?.name)}ButtonType"`}
(filesChange)= ${nameStringToVariableString(json.codeContext?.name)}onDropped.emit($event)>
Copy link
Member

Choose a reason for hiding this comment

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

name should be camelCase

sdk/react/src/lib/fragment-components/a-file-uploader.tsx
Comment on lines +290 to +305
code: ({ json }) => {
return `<ibm-file-uploader
${angularClassNamesFromComponentObj(json)}
[title]="${nameStringToVariableString(json.codeContext?.name)}Title"
[description]="${nameStringToVariableString(json.codeContext?.name)}Description"
[accept]="${nameStringToVariableString(json.codeContext?.name)}Accept"
[multiple]="${nameStringToVariableString(json.codeContext?.name)}Multiple"
[disabled]="${nameStringToVariableString(json.codeContext?.name)}Disabled"
[size]="${nameStringToVariableString(json.codeContext?.name)}Size"
[disabled]="${nameStringToVariableString(json.codeContext?.name)}Disabled"
[drop]="${nameStringToVariableString(json.codeContext?.name)}Drop"
${json.dragAndDrop ? `[dropText]="${nameStringToVariableString(json.codeContext?.name)}DropText"` : ''}
${json.dragAndDrop ? '' : `[buttonText]="${nameStringToVariableString(json.codeContext?.name)}ButtonText"`}
${json.dragAndDrop ? '' : `[buttonType]="${nameStringToVariableString(json.codeContext?.name)}ButtonType"`}
(filesChange)= ${nameStringToVariableString(json.codeContext?.name)}onDropped.emit($event)>
</ibm-file-uploader>`;
Copy link
Member

Choose a reason for hiding this comment

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

indentation + it doesn't need the explicit return statement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zvonimirfras zvonimirfras zvonimirfras requested changes

Assignees
No one assigned
Labels
None yet
Projects
Carbon UI Builder
Status: QA/Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maxxyouu @zvonimirfras