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(web-twig): Introduce advanced Tooltip usage supported by FloatingUI #1246

Merged
merged 4 commits into from
Jan 29, 2024
Merged

Feat(web-twig): Introduce advanced Tooltip usage supported by FloatingUI #1246

merged 4 commits into from
Jan 29, 2024

Conversation

literat
Copy link
Collaborator

@literat literat commented Jan 22, 2024 •
edited
Loading

Description

Integrating advanced Tooltip usage and advanced floating ability into web-twig Tooltip component.

Additional context

Created a new TooltipPopover component with defined features. The same way we do this in the web-react. So the the TooltipWrapper will be renamed to Tooltip to stay compatible with implementation.

Issue reference

https://jira.lmc.cz/browse/DS-1087

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Follow the PR Title/Commit Message Convention.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@netlify Netlify
Copy link

netlify bot commented Jan 22, 2024 •
edited
Loading

✅ Deploy Preview for spirit-design-system-demo canceled.

Name Link
🔨 Latest commit 42a5f07
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-demo/deploys/65b772fcddb4ec0008e2fbd9

@netlify Netlify
Copy link

netlify bot commented Jan 22, 2024 •
edited
Loading

✅ Deploy Preview for spirit-design-system-react canceled.

Name Link
🔨 Latest commit 42a5f07
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-react/deploys/65b772fce0269a0008a7db2f

@github-actions github-actions bot added the feature New feature or request label Jan 22, 2024
@literat literat force-pushed the feat/DS-10287-web-twig-tooltip-floating-ui branch from 4c354c2 to 4bad6f1 Compare January 22, 2024 13:24
@github-actions GitHub Actions
Copy link
Contributor

Coverage Status

coverage: 72.613% (-5.7%) from 78.281%
when pulling 4bad6f1 on feat/DS-10287-web-twig-tooltip-floating-ui
into f5f004f on feat/DS-1086-fui-integration.

@coveralls
Copy link

coveralls commented Jan 22, 2024 •
edited
Loading

Coverage Status

coverage: 80.912% (+0.01%) from 80.901%
when pulling 42a5f07 on feat/DS-10287-web-twig-tooltip-floating-ui
into ece47b5 on integration/tooltip.

@literat literat force-pushed the feat/DS-10287-web-twig-tooltip-floating-ui branch 2 times, most recently from f9da091 to 5ba23d0 Compare January 22, 2024 14:04
@literat literat marked this pull request as ready for review January 22, 2024 15:46
adamkudrna
adamkudrna reviewed Jan 22, 2024
View reviewed changes
Copy link
Contributor

@adamkudrna adamkudrna left a comment

Choose a reason for hiding this comment

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

Just a quick review. I have not seen it live nor checked it against the React implementation.

packages/web-twig/src/Resources/components/Tooltip/TooltipWrapper.twig Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/README.md Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/README.md Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/README.md Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/README.md Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/Tooltip.twig Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/stories/TooltipDismissibleViaJS.twig Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/stories/TooltipWithJsPlugin.twig Show resolved Hide resolved
apps/web-twig-demo/package.json Show resolved Hide resolved
apps/web-twig-demo/assets/scripts/tooltip-advanced-usage.ts Outdated Show resolved Hide resolved
@literat
Copy link
Collaborator Author

literat commented Jan 23, 2024

🔝 @adamkudrna some of the suggestions need your answer, so please, give this a second try. Thanks.

@literat literat mentioned this pull request Jan 23, 2024
Closed
@pavelklibani pavelklibani force-pushed the feat/DS-1086-fui-integration branch from f5f004f to e69f597 Compare January 25, 2024 10:13
crishpeen
crishpeen previously requested changes Jan 25, 2024
View reviewed changes
Copy link
Member

@crishpeen crishpeen left a comment

Choose a reason for hiding this comment

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

image

:( not working for me.

Also please you need to rebase over main branch. There are updates in Tooltip component.

apps/web-twig-demo/package.json Outdated Show resolved Hide resolved
apps/web-twig-demo/package.json Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/README.md Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/README.md Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/README.md Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/Tooltip.twig Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/Tooltip.twig Outdated Show resolved Hide resolved
...ges/web-twig/src/Resources/components/Tooltip/__tests__/__snapshots__/tooltip.twig.snap.html Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/stories/TooltipClickable.twig Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Tooltip/stories/TooltipWithJsPlugin.twig Show resolved Hide resolved
Base automatically changed from feat/DS-1086-fui-integration to integration/tooltip January 25, 2024 10:22
@literat literat force-pushed the feat/DS-10287-web-twig-tooltip-floating-ui branch from cd2da86 to 8ad4185 Compare January 25, 2024 14:57
@literat
Copy link
Collaborator Author

literat commented Jan 25, 2024 •
edited
Loading

  • Add TooltipModer to readme

@literat
Copy link
Collaborator Author

literat commented Jan 26, 2024

image

:( not working for me.

Also please you need to rebase over main branch. There are updates in Tooltip component.

This has been fixed with the rebase.

@literat literat force-pushed the feat/DS-10287-web-twig-tooltip-floating-ui branch from b608593 to 59f4252 Compare January 26, 2024 13:23
@literat literat force-pushed the feat/DS-10287-web-twig-tooltip-floating-ui branch from a995212 to 42a5f07 Compare January 29, 2024 09:42
@literat literat dismissed crishpeen’s stale review January 29, 2024 09:43

This was solved by rebase.

@literat literat merged commit 89f7ebc into integration/tooltip Jan 29, 2024
27 checks passed
@literat literat deleted the feat/DS-10287-web-twig-tooltip-floating-ui branch January 29, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavelklibani pavelklibani pavelklibani approved these changes

@adamkudrna adamkudrna Awaiting requested review from adamkudrna

@crishpeen crishpeen Awaiting requested review from crishpeen

Assignees

@literat literat

Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@literat @coveralls @adamkudrna @crishpeen @pavelklibani