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 inline notification component #301

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

feat: add inline notification component #301

wants to merge 35 commits into from

Conversation

maxxyouu
Copy link
Contributor

@maxxyouu maxxyouu commented Feb 29, 2024 •
edited
Loading

for #159

initial commit
3f21be1 
Signed-off-by: Max You <[email protected]>
@netlify Netlify
Copy link

netlify bot commented Feb 29, 2024 •
edited
Loading

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit 06d5121

Max You added 5 commits February 29, 2024 14:45
conflict resolved
705f81d 
Signed-off-by: Max You <[email protected]>
resolved
4e41a90 
Signed-off-by: Max You <[email protected]>
resolved
2ed3ef4 
Signed-off-by: Max You <[email protected]>
rename the file
d3f6bb3 
Signed-off-by: Max You <[email protected]>
fixed
2e197c4 
Signed-off-by: Max You <[email protected]>
@maxxyouu
Copy link
Contributor Author

Verified that the component features and export works.

Max You added 4 commits April 18, 2024 12:40
export
ba00755 
Signed-off-by: Max You <[email protected]>
action
2f7f2c6 
Signed-off-by: Max You <[email protected]>
Merge branch 'main' of https://github.com/IBM/carbon-ui-builder into …
2cf064f 
…notification_clean
lint
b297629 
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.

  • notification and toast are separate components in both implementations, why stuff them into the same component here?

  • styling with the ternary operators is all over the place

player/react/src/lib/components/ui-notification.tsx Outdated
id: string;
type: string;
lowContrast?: boolean;
hideCloseButton?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

property name that sounds like a function

player/react/src/lib/components/ui-notification.tsx Outdated
...commonSlots,
...slotsDisabled,
lowContrast: 'boolean',
isLowContrast: (state: NotificationState) => ({
Copy link
Member

@zvonimirfras zvonimirfras Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

naming

player/react/src/lib/components/ui-notification.tsx Outdated
Comment on lines 62 to 64
link: 'string',
subtitleText: 'string',
linkText: 'string',
Copy link
Member

Choose a reason for hiding this comment

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

link vs linkText... might need clearer naming

player/react/src/lib/components/ui-notification.tsx Outdated
linkText: 'string',
title: 'string',
iconDescription: 'string',
captionText: 'string'
Copy link
Member

Choose a reason for hiding this comment

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

why not just caption?

player/react/src/lib/components/ui-notification.tsx Outdated
Comment on lines 83 to 105
state.variant === 'toastNotification'
? <ToastNotification
className={state.cssClasses?.map((cc: any) => cc.id).join(' ')}
onClick={() => {
sendSignal(state.id, 'click');
}}
onChange={(event: any) => {
sendSignal(state.id, 'valueChange', [event.value], { ...state, value: event.value });
}}
caption={state.captionText}
iconDescription={state.iconDescription}
hideCloseButton={state.hideCloseButton}
lowContrast={state.lowContrast}
kind={state.kind}
subtitle={state.subtitleText}
timeout={0}
title={state.title} />
: <InlineNotification
className={state.cssClasses?.map((cc: any) => cc.id).join(' ')}
kind={state.kind}
iconDescription={state.iconDescription}
subtitle= {state.subtitleText}
title={state.title} />
Copy link
Member

Choose a reason for hiding this comment

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

indentation

sdk/react/src/lib/fragment-components/a-notification.tsx Outdated Show resolved Hide resolved
sdk/react/src/lib/fragment-components/a-notification.tsx Outdated
Comment on lines 100 to 107
selectedComponent.variant === 'toastNotification' && <TextInput
light
value={selectedComponent.captionText}
labelText='Caption text'
onChange={(event: any) => setComponent({
...selectedComponent,
captionText: 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.

indentation and styling

sdk/react/src/lib/fragment-components/a-notification.tsx Outdated
Comment on lines 139 to 155
componentObj.variant === 'toastNotification'
? <ToastNotification
className={cx(preventCheckEventStyle, componentObj.cssClasses?.map((cc: any) => cc.id).join(' '))}
caption={componentObj.captionText}
hideCloseButton={componentObj.hideCloseButton}
lowContrast={componentObj.lowContrast}
kind={componentObj.kind}
subtitle={componentObj.subtitleText}
timeout={0}
title={componentObj.title} />
: <InlineNotification
className={cx(preventCheckEventStyle, componentObj.cssClasses?.map((cc: any) => cc.id).join(' '))}
kind={componentObj.kind}
hideCloseButton={componentObj.hideCloseButton}
lowContrast={componentObj.lowContrast}
subtitle={componentObj.subtitleText}
title={componentObj.title} />
Copy link
Member

Choose a reason for hiding this comment

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

indentation

sdk/react/src/lib/fragment-components/a-notification.tsx Outdated
Comment on lines 191 to 198
? `<cds-toast
${angularClassNamesFromComponentObj(json)}
[notificationObj]="${nameStringToVariableString(json.codeContext?.name)}notificationObj">
</cds-toast>`
: `<cds-inline-notification
${angularClassNamesFromComponentObj(json)}
[notificationObj]="${nameStringToVariableString(json.codeContext?.name)}notificationObj">
</cds-inline-notification>`
Copy link
Member

Choose a reason for hiding this comment

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

indentation

sdk/react/src/lib/fragment-components/a-notification.tsx Outdated
Comment on lines 208 to 211
${json.variant === 'toastNotification' ?
`caption: "${json.captionText ? json.captionText : ''}",` : `message: "${json.captionText ? json.captionText : ''}",`}
lowContrast:${json.lowContrast ? json.lowContrast : false},
showClose: ${!json.hideCloseButton}
Copy link
Member

Choose a reason for hiding this comment

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

hard to read

@maxxyouu maxxyouu changed the title feat: add notification component feat: add inline notification component Jul 25, 2024
Max You added 2 commits July 25, 2024 13:05
revision
9614869 
Signed-off-by: Max You <[email protected]>
revision
12e12a5 
Signed-off-by: Max You <[email protected]>
Max You added 3 commits July 25, 2024 13:07
revision
67db84f 
Signed-off-by: Max You <[email protected]>
with icon description
601574d 
Signed-off-by: Max You <[email protected]>
add low contrast and hide close button
da9eea3 
Signed-off-by: Max You <[email protected]>
zvonimirfras
zvonimirfras requested changes Aug 1, 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.

Should it be notification instead of inline-notification? 🤔

cc: @Akshat55

player/react/src/lib/components/index.ts Outdated Show resolved Hide resolved
sdk/react/src/lib/components/actions-connector.tsx Outdated Show resolved Hide resolved
sdk/react/src/lib/fragment-components/a-inline-notification.tsx Outdated Show resolved Hide resolved
sdk/react/src/lib/fragment-components/index.ts Outdated Show resolved Hide resolved
sdk/react/src/lib/fragment-components/a-inline-notification.tsx Outdated Show resolved Hide resolved
zvonimirfras
zvonimirfras requested changes Aug 8, 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.

Should subtitle be caption?

player/react/src/lib/components/ui-inline-notification.tsx Outdated Show resolved Hide resolved
sdk/react/src/lib/fragment-components/a-inline-notification.tsx Outdated Show resolved Hide resolved
sdk/react/src/lib/fragment-components/a-inline-notification.tsx Outdated Show resolved Hide resolved
@maxxyouu
Copy link
Contributor Author

Should subtitle be caption?

no

pr revision
06d5121 
Signed-off-by: Max You <[email protected]>
@zvonimirfras
Copy link
Member

Should subtitle be caption?

no

please explain

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: In progress
Milestone
No milestone
Development

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