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.

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

feat: add popover as a fragment component #304

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

Conversation

jz5426
Copy link
Contributor

@jz5426 jz5426 commented Apr 10, 2024

Max You added 3 commits April 10, 2024 13:37
Signed-off-by: Max You <[email protected]>
Signed-off-by: Max You <[email protected]>
Signed-off-by: Max You <[email protected]>
Copy link

netlify bot commented Apr 10, 2024

Deploy Preview for carbon-components-builder ready!

Name Link
🔨 Latest commit 935b1d2
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-builder/deploys/663e2b146073ac0008324ea0
😎 Deploy Preview https://deploy-preview-304--carbon-components-builder.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Max You added 2 commits April 26, 2024 16:11
Signed-off-by: Max You <[email protected]>
Signed-off-by: Max You <[email protected]>
@jz5426
Copy link
Contributor Author

jz5426 commented Apr 26, 2024

Verified that the component features and export works.

Comment on lines 130 to 133
<svg _ngcontent-pla-c12="" preserveAspectRatio="xMidYMid meet" xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 32 32">
<path _ngcontent-pla-c12="" d="M26,4H6A2,2,0,0,0,4,6V26a2,2,0,0,0,2,2H26a2,2,0,0,0,2-2V6A2,2,0,0,0,26,4ZM6,26V6H26V26Z">
</path>
</svg>
Copy link
Member

Choose a reason for hiding this comment

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

You're hard coding icon here.

Ideally, we would want to allow the user to specify the tooltip trigger... so not sure what the best way to approach this is. I guess we can have an icon as an option here? @zvonimirfras @maxxyouu

Comment on lines 154 to 159
keywords: ['popover'],
name: 'Popover',
type: 'popover',
defaultComponentObj: {
type: 'popover',
isOpen: true
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a text attribute and assign it a value, this would make the tooltip appear correctly instead of appearing like this:

image

Comment on lines 204 to 244
v10: {
inputs: ({ json }) => `
@Input() ${nameStringToVariableString(json.codeContext?.name)}IsOpen = ${json.isOpen};
@Input() ${nameStringToVariableString(json.codeContext?.name)}IsShowCaret = ${json.isShowCaret};
@Input() ${nameStringToVariableString(json.codeContext?.name)}IsDropShadow = ${json.isDropShadow};
@Input() ${nameStringToVariableString(json.codeContext?.name)}IsHighContrast = ${json.isHighContrast};
@Input() ${nameStringToVariableString(json.codeContext?.name)}Align: any = '${json.align ? json.align : 'bottom'}';
@Input() ${nameStringToVariableString(json.codeContext?.name)}Text = '${json.text}';
`,
outputs: ({ json }) => `
@Output() ${nameStringToVariableString(json.codeContext?.name)}OnOpen = new EventEmitter();
@Output() ${nameStringToVariableString(json.codeContext?.name)}OnClose = new EventEmitter();
@Output() ${nameStringToVariableString(json.codeContext?.name)}IsOpenChange = new EventEmitter();`,
imports: ['PopoverModule'],
code: ({ json }) => {
return `<div
cdsPopover
${angularClassNamesFromComponentObj(json)}
[isOpen]="${nameStringToVariableString(json.codeContext?.name)}IsOpen"
[dropShadow]="${nameStringToVariableString(json.codeContext?.name)}IsDropShadow"
[align]="${nameStringToVariableString(json.codeContext?.name)}Align"
[caret]="${nameStringToVariableString(json.codeContext?.name)}IsShowCaret"
[highContrast]="${nameStringToVariableString(json.codeContext?.name)}IsHighContrast"
(onOpen)="${nameStringToVariableString(json.codeContext?.name)}OnOpen.emit($event)"
(onClose)="${nameStringToVariableString(json.codeContext?.name)}onClose.emit($event)"
(isOpenChange)="${nameStringToVariableString(json.codeContext?.name)}IsOpenChange.emit($event)">
<div class="popover-trigger">
<svg preserveAspectRatio="xMidYMid meet" xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 32 32">
<path d="M26,4H6A2,2,0,0,0,4,6V26a2,2,0,0,0,2,2H26a2,2,0,0,0,2-2V6A2,2,0,0,0,26,4ZM6,26V6H26V26Z"></path>
</svg>
</div>
<cds-popover-content>
<div style="padding: 1rem">
${ json.text }
</div>
</cds-popover-content>
</div>
`;
}
}
},
Copy link
Member

Choose a reason for hiding this comment

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

Leave v10 input and output as blank. This component did NOT exist in v10.

Comment on lines 263 to 279
v10: {
imports: ['Popover', 'PopoverContent'],
code: ({ json }) => {
return `<Popover
open={${json.isOpen ? json.isOpen : false}}
align={${json.align ? json.align : (json.isTabTip ? 'bottom-start' : 'bottom')}}
caret={${json.isShowCaret ? json.isShowCaret : (json.isTabTip ? false : true)}}
dropShadow={${json.isDropShadow ? json.isDropShadow : true}}
highContrast={${json.isHighContrast ? json.isHighContrast : true}}
${json.isTabTip ? 'isTabTip' : ''}
${reactClassNamesFromComponentObj(json)}>
<PopoverContent>
${ json.text }
</PopoverContent>
</Popover>`;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Leave v10 input and output as blank. This component did NOT exist in v10.

}
}
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Missing new line

Comment on lines 22 to 27
const alignItems = [
{id: 'top', text: 'Top'},
{id: 'right', text: 'Right'},
{id: 'bottom', text: 'Bottom'},
{id: 'left', text: 'Left'}
];
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines 144 to 146
component: APopOver,
settingsUI: APopOverSettingsUI,
codeUI: APopOverCodeUI,
Copy link
Member

Choose a reason for hiding this comment

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

Should be APopover, fix all instances,

Suggested change
component: APopOver,
settingsUI: APopOverSettingsUI,
codeUI: APopOverCodeUI,
component: APopover,
settingsUI: APopoverSettingsUI,
codeUI: APopoverCodeUI,

codeExport: {
angular: {
latest: {
inputs: ({ json }) => `
Copy link
Member

Choose a reason for hiding this comment

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

Extra indentation? Have you ran the linter?

Comment on lines 219 to 220
return `<div
cdsPopover
Copy link
Member

Choose a reason for hiding this comment

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

Identation - run linter

Max You added 7 commits May 2, 2024 15:50
@Akshat55 Akshat55 changed the title Popover feat: add definition tooltip component May 22, 2024
@Akshat55 Akshat55 changed the title feat: add definition tooltip component feat: add definition tooltip component as a fragment component May 22, 2024
@Akshat55 Akshat55 changed the title feat: add definition tooltip component as a fragment component feat: add popoverp component as a fragment component May 22, 2024
@Akshat55 Akshat55 changed the title feat: add popoverp component as a fragment component feat: add popover component as a fragment component May 22, 2024
@Akshat55 Akshat55 changed the title feat: add popover component as a fragment component feat: add popover as a fragment component May 22, 2024
@zvonimirfras zvonimirfras requested a review from Akshat55 July 11, 2024 08:59
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.

indentation of many files here is a bit all over the place

Let's fix that before moving forward, it's hard to read at the moment.

Comment on lines +125 to +130
open={state.isOpen ? state.isOpen : false}
align={state.align ? state.align : (state.isTabTip ? 'bottom-start' : 'bottom')}
caret={state.isShowCaret ? state.isShowCaret : (state.isTabTip ? false : true)}
dropShadow={state.isDropShadow ? state.isDropShadow : true}
highContrast={state.isHighContrast ? state.isHighContrast : true}
isTabTip={state.isTabTip ? state.isTabTip : false}>
Copy link
Member

Choose a reason for hiding this comment

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

indentation

import { styleObjectToString } from '@carbon-builder/player-react';

export const APopoverSettingsUI = ({ selectedComponent, setComponent }: any) => {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

{ id: 'right-start', text: 'Right start' }
];

return <>
Copy link
Member

Choose a reason for hiding this comment

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

indentation of the whole return

type: 'popover',
defaultComponentObj: {
type: 'popover',
label: 'popover',
Copy link
Member

Choose a reason for hiding this comment

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

Needs a better label

defaultComponentObj: {
type: 'popover',
label: 'popover',
isOpen: true
Copy link
Member

Choose a reason for hiding this comment

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

Why is this default?

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.

3 participants