Skip to content

Conversation

@jz5426
Copy link
Contributor

@jz5426 jz5426 commented Mar 7, 2024

for #184

Max You added 3 commits March 6, 2024 22:16
Signed-off-by: Max You <JianzhongMax.You@ibm.com>
Signed-off-by: Max You <JianzhongMax.You@ibm.com>
Signed-off-by: Max You <JianzhongMax.You@ibm.com>
@netlify
Copy link

netlify bot commented Mar 7, 2024

👷 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
Signed-off-by: Max You <JianzhongMax.You@ibm.com>
Signed-off-by: Max You <JianzhongMax.You@ibm.com>
@jz5426
Copy link
Contributor Author

jz5426 commented Apr 18, 2024

Verified that the component features and export works.

Max You added 3 commits April 19, 2024 12:23
Signed-off-by: Max You <JianzhongMax.You@ibm.com>
Signed-off-by: Max You <JianzhongMax.You@ibm.com>
@Akshat55 Akshat55 changed the title feat: file uploader feat: add file uploader as a fragment component May 22, 2024
Signed-off-by: Max You <JianzhongMax.You@ibm.com>
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

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

...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

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

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

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

...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

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)

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??

${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

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

Labels

None yet

Projects

Status: QA/Review

Development

Successfully merging this pull request may close these issues.

2 participants