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

maxxyouu
Copy link
Contributor

@maxxyouu maxxyouu 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

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

@maxxyouu maxxyouu force-pushed the dev.tooltip branch 2 times, most recently from 794108b to da47417 Compare June 8, 2023 14:06
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]>
@maxxyouu
Copy link
Contributor Author

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