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 tooltip element to the builder #246

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

Conversation

jz5426
Copy link
Contributor

@jz5426 jz5426 commented May 18, 2023

closes #132

@netlify
Copy link

netlify bot commented May 18, 2023

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit 2462361

@jz5426 jz5426 marked this pull request as ready for review May 19, 2023 12:06
@jz5426 jz5426 changed the title fea: add tooltip elements to the builder feat: add tooltip elements to the builder May 19, 2023
@jz5426 jz5426 changed the title feat: add tooltip elements to the builder feat: add tooltip element to the builder May 19, 2023
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.

Use the icon from here

@jz5426 jz5426 force-pushed the dev.tooltip branch 2 times, most recently from 794108b to da47417 Compare June 8, 2023 14:06
@jz5426 jz5426 requested a review from zvonimirfras June 8, 2023 14:20
src/sdk/src/fragment-components/a-tooltip.tsx Outdated Show resolved Hide resolved
<AComponent
componentObj={componentObj}
rejectDrop={true}
// className={css`position: relative; display: inline-flex`}
Copy link
Member

Choose a reason for hiding this comment

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

?

select={select}
remove={remove}
selected={selected}>

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

?

Also, if there's no children, it should be a self-closing tag, no? <Tooltip ... />

Comment on lines 125 to 126
placement: 'top',
alignment: 'center',
Copy link
Member

Choose a reason for hiding this comment

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

are these default values? if yes, we don't (shouldn't) need to specify them, if bloats the json.

outputs: () => '',
imports: ['TooltipModule'],
code: ({ json }) => {
return `<div class="bx--tooltip__label">
Copy link
Member

Choose a reason for hiding this comment

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

can be inline (without the return statement)?

react: {
imports: ['Tooltip'],
code: ({ json }) => {
return `<Tooltip
Copy link
Member

Choose a reason for hiding this comment

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

can be inline (without the return statement)?

return <Tooltip
description={state.description}
direction={state.placement ? state.placement : 'top'}
align={state.alignment ? state.alignment : 'center'}
Copy link
Member

Choose a reason for hiding this comment

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

indentation

}
return <Tooltip
description={state.description}
direction={state.placement ? state.placement : 'top'}
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 it "placement" if the property it's used for is "direction"?
Also, how will this change in v11?

Max You added 6 commits June 9, 2023 17:42
Signed-off-by: Max You <[email protected]>
Signed-off-by: Max You <[email protected]>
svg
Signed-off-by: Max You <[email protected]>
svg
Signed-off-by: Max You <[email protected]>
Signed-off-by: Max You <[email protected]>
Signed-off-by: Max You <[email protected]>
@jz5426
Copy link
Contributor Author

jz5426 commented Apr 25, 2024

Verified that the component features and export works.

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.

input and output names should be camelCase

player/react/src/lib/components/ui-definition-tooltip.tsx Outdated Show resolved Hide resolved
player/react/src/lib/components/ui-definition-tooltip.tsx Outdated Show resolved Hide resolved
className={css`${styleObjectToString(componentObj.style)}`}
definition={componentObj.definition}
align={componentObj.alignment}>
{componentObj.description}
Copy link
Member

Choose a reason for hiding this comment

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

In both frameworks description is a child, so why don't we do that here too? Meaning description wouldn't be part of the componentObj.description - instead we just let items render.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we stick with support only text only for now

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.

Add Tooltip
2 participants