update tooltip component based on new DS #10974
Conversation
amit-ksh
requested review from
samajammin,
wackerow,
corwintines,
pettinarip,
minimalsm,
jmcook1186 and
nhsz
as code owners
✅ ethereum-org-website-dev deploy preview ready
|
|
Hey @amit-ksh! In order for this to be properly reviewed and approved, any component new or existing that is directly affected by the new DS needs to have at least one storybook story (unless otherwise), and following a proposed Storybook structure outline in this figma file. This component is under Follow the Applying Storybook documentation and let us know if you have any further questions. 😄 I will still help review what you currently have. 👍🏼 |
There was a problem hiding this comment.
Here are my thoughts so far! 😄
Primarily, I believe the feature branch here should be updated to the latest version of dev and looked over.
There is a new GlossaryTooltip component that is in prod. It is now clear compared to the Figma file that the tooltip component's usage is for specific text in body copy as well as with the info icon that is rendered with content like short statistical information.
I wonder then, if this should be renamed InfoTooltip and reworked so that it only renders the info icon?
If this can be better established, then can comment further on styling issues involving the inner wrapper (the Center component).
cc @pettinarip what do you think?
Co-authored-by: Tyler Pfledderer <[email protected]>
Co-authored-by: Tyler Pfledderer <[email protected]>
Co-authored-by: Tyler Pfledderer <[email protected]>
There was a problem hiding this comment.
Thanks @amit-ksh 💪🏼
While I was reviewing this I noticed that the Tooltip from Chakra might not be the ideal component to use for our tooltips since we use them a bit differently.
- we want to put rich content inside the tooltip (like links), not just plain text as it is expected in the Chakra tooltip
- we want to keep it open when the user hovers over the tooltip content
That behavior sounds like the Popover is a better fit for this use case. Should be used like this:
<Popover trigger="hover">
...
</Popover>@TylerAPfledderer what do you think of that? or is it possible to control the Tooltip and achieve something like that?
============
Another thing to mention is that we probably don't need to create a new component for this. We probably just need to create a theme file to change the default styles to the ones we want from the DS.
We can keep the My idea is to create this "base" tooltip with the base styles and then refactor the Also, this Tooltip should be able to be used anywhere, in any context. I mean, not only on body copy or icons, we should wrap a Card with the tooltip if we would like. Note: perhaps we should start calling this Popover rather than Tooltip :P |
|
@pettinarip ah, right. Yea the popover is needed to be able to navigate to that rich content via keyboard. |
Co-authored-by: Tyler Pfledderer <[email protected]>
There was a problem hiding this comment.
Looking good @amit-ksh. I've added a few minor comments.
I think that overall, the new component looks good but is not ready to be used in all the pages we have. We need to tweak a few things from the consumer's side before we can use the new Tooltip.
- For that reason, lets revert the
src/components/Tooltip.tsxremoval from this PR.
In a different issue/PR, we will do the transition from the old tooltip to the new tooltip, which is going to involve some style adjustments + more tests across the different pages that are involved.
Co-authored-by: Pablo Pettinari <[email protected]>
Co-authored-by: Pablo Pettinari <[email protected]>
|
Reviving this PR. I've fixed the tooltip theme config to be consistent with all the tooltips we have across the site. @konopkja this PR fixes the positioning & zindex issues we have with the current custom component. Pages using the Tooltip: |
WalkthroughThe recent updates introduce a redesigned Tooltip component and integrate it across the platform, leveraging the new Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (15)
- src/@chakra-ui/components/Popover.ts (1 hunks)
- src/@chakra-ui/components/components.utils.ts (2 hunks)
- src/@chakra-ui/components/index.ts (2 hunks)
- src/components/DencunBanner.tsx (2 hunks)
- src/components/EthPriceCard.tsx (1 hunks)
- src/components/FindWallet/WalletTable/WalletMoreInfoCategory.tsx (1 hunks)
- src/components/Simulator/MoreInfoPopover.tsx (1 hunks)
- src/components/Simulator/NotificationPopover.tsx (1 hunks)
- src/components/Staking/StakingStatsBox.tsx (5 hunks)
- src/components/StatsBoxGrid/GridItem.tsx (3 hunks)
- src/components/Tooltip/Tooltip.stories.tsx (1 hunks)
- src/components/Tooltip/index.tsx (1 hunks)
- src/pages/stablecoins.tsx (3 hunks)
- src/pages/staking/deposit-contract.tsx (3 hunks)
- src/pages/what-is-ethereum.tsx (6 hunks)
Additional comments: 22
src/components/Tooltip/index.tsx (2)
- 11-13: The
IPropsinterface extendsPopoverPropsand adds acontentprop for the tooltip content and an optionalchildrenprop. This is a clear and concise way to define the component's expected props, ensuring that all necessary Chakra UIPopoverPropscan be passed through while also specifying the additional props required by this custom Tooltip component.- 16-36: The Tooltip component is correctly implemented using Chakra UI's Popover components. It sets default properties such as
placement,trigger, andstrategyto ensure consistent behavior across uses. The use of rest parameters (...rest) to pass any additionalPopoverPropsis a good practice, allowing for flexibility in the component's usage. However, consider adding PropTypes or TypeScript definitions for better type safety and documentation, especially for projects that heavily rely on component libraries.src/components/Simulator/NotificationPopover.tsx (3)
- 29-29: The
PopoverContentcomponent's padding and maximum width have been adjusted. This simplification aligns with the aim to standardize the component's appearance. However, ensure that these changes do not adversely affect the layout or readability in different contexts where theNotificationPopovermight be used.- 31-31: Adjusting the
PopoverHeader's margin and padding is a good practice for maintaining consistency in the component's design. It's important to verify that these changes harmonize with the overall design system and do not introduce any unexpected layout shifts.- 36-36: Simplifying the
PopoverBodypadding to0might affect the content's readability and appearance. Consider testing this change across different screen sizes and content types to ensure it maintains a good user experience.src/components/DencunBanner.tsx (2)
- 9-9: The import statement correctly switches from using a Tooltip component from
@chakra-ui/reactto the newly defined local Tooltip component. This change is necessary to align with the PR's objective of updating the Tooltip component according to the new design system.- 21-24: The updated usage of the Tooltip component with a
contentprop instead oflabelis in line with the new component's API. Wrapping theupgradeDatewith aspanelement inside the Tooltip trigger is a good practice for accessibility and semantic HTML. Ensure that thearia-labelproperty, which was previously used, is appropriately handled or migrated to the new implementation to maintain accessibility standards.src/components/Tooltip/Tooltip.stories.tsx (1)
- 51-51: Given the existing comments and responses, it appears that the request for a new story showcasing rich content usage has been addressed. This is a good practice for demonstrating the component's capabilities and ensuring it meets the design requirements.
src/@chakra-ui/components/index.ts (1)
- 22-22: Adding the
Popovercomponent to the list of exported components from@chakra-ui/componentsis a necessary step to ensure it can be used throughout the project. This change aligns with the PR's objectives of updating the Tooltip component and leveraging Chakra UI components more extensively.src/components/EthPriceCard.tsx (1)
- 144-146: The modification to wrap the tooltip icon in a
Boxcomponent is a good practice for applying additional styling or layout control in Chakra UI. Ensure that the visual outcome aligns with the new design system's requirements.src/components/StatsBoxGrid/GridItem.tsx (1)
- 35-57: The layout adjustments using
BoxandHStackcomponents, along with the modifications to the tooltip icon's styling, are well-thought-out changes that likely aim to enhance the component's usability and visual appeal. Ensure these changes are consistent with the new design system and consider accessibility implications.src/pages/staking/deposit-contract.tsx (1)
- 135-143: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [138-150]
The use of
forwardRefin theAddresscomponent is correctly implemented and enhances the component's flexibility by allowing direct interaction with the DOM element. Ensure that the necessity and intended use cases for this feature are documented to avoid confusion or misuse.src/pages/stablecoins.tsx (4)
- 239-244: The change from
<div>to<Box>fortooltipContentaligns well with the Chakra UI framework's approach, ensuring consistency in component usage throughout the project. Good job on maintaining design system consistency.- 239-239: Adjusting the indentation for
{t("common:data-provided-by")}improves code readability and adheres to consistent formatting practices. It's important to maintain such standards for code quality.- 605-605: Correcting the indentation for
{t("page-stablecoins-algorithmic-disclaimer")}is a good practice for maintaining code quality and readability. Consistent formatting is key in collaborative projects.- 734-734: Removing extra whitespace before
<Flex>contributes to a cleaner and more readable codebase. It's great to see attention to detail in maintaining clean code practices.src/pages/what-is-ethereum.tsx (6)
- 482-484: The change to wrap the
<Icon>component in a<Box>withas="span"andverticalAlign="middle"is a good improvement. It enhances both the semantic structure and the visual alignment of the icon with adjacent text.- 503-505: This instance of wrapping the
<Icon>component in a<Box>withas="span"andverticalAlign="middle"maintains consistency and improves the page's semantic structure and visual alignment. Good job on applying these improvements consistently.- 524-526: Consistent with previous instances, wrapping the
<Icon>component in a<Box>withas="span"andverticalAlign="middle"here as well ensures uniformity and enhances the page's semantic markup and visual presentation.- 544-546: Again, wrapping the
<Icon>component in a<Box>withas="span"andverticalAlign="middle"is applied consistently, enhancing the semantic structure and alignment of the icon. This consistent application across the page is commendable.- 566-568: The consistent application of wrapping the
<Icon>component in a<Box>withas="span"andverticalAlign="middle"here as well further solidifies the improvements in semantic structure and visual alignment across the page.- 590-592: Wrapping the
<Icon>component in a<Box>withas="span"andverticalAlign="middle"for this final instance maintains the high standard of consistency and quality seen throughout the page. Excellent attention to detail in enhancing both the semantic and visual aspects.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- src/@chakra-ui/components/Popover.ts (1 hunks)
- src/components/Staking/StakingStatsBox.tsx (5 hunks)
- src/components/StatsBoxGrid/GridItem.tsx (3 hunks)
- src/components/Tooltip/index.tsx (1 hunks)
- src/pages/stablecoins.tsx (2 hunks)
- src/pages/staking/deposit-contract.tsx (3 hunks)
- src/pages/what-is-ethereum.tsx (6 hunks)
Files skipped from review as they are similar to previous changes (7)
- src/@chakra-ui/components/Popover.ts
- src/components/Staking/StakingStatsBox.tsx
- src/components/StatsBoxGrid/GridItem.tsx
- src/components/Tooltip/index.tsx
- src/pages/stablecoins.tsx
- src/pages/staking/deposit-contract.tsx
- src/pages/what-is-ethereum.tsx
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/@chakra-ui/components/Popover.ts (1 hunks)
- src/components/FindWallet/WalletTable/WalletMoreInfoCategory.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/@chakra-ui/components/Popover.ts
- src/components/FindWallet/WalletTable/WalletMoreInfoCategory.tsx
…when tooltip is displayed
|
Congrats, your important contribution to this open-source project has earned you a GitPOAP! Be sure to join the Ethereum.org discord if you are interested in contributing further to the project or have any questions for the team. GitPOAP: 2024 Ethereum.org Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
|
@pettinarip i noticed in the PR that on find-wallet page some tooltips adopt wrong styles. What should we do with this? |
I see. I'll create a fix for this soon 👍🏼 |
Description
Updated the tooltip component according to the new design system using the Chakra Tooltip component.
Related Issue
#9548
Closes #10747
Summary by CodeRabbit
New Features
Popovercomponent for Chakra UI with customizable styles and configurations.Tooltipcomponent that displays information on hover, utilizing the Chakra UIPopover.Enhancements
DencunBanner,EthPriceCard,WalletMoreInfoCategory,Simulator,StakingStatsBox,GridItem) for improved layout and styling, including better use of Chakra UI components likeBox,Icon, and tooltips.Tooltipcomponent's content rendering and configuration options.Code Quality
Simulatorcomponents for cleaner code and better user experience.forwardRefinAddresscomponent for improved component reference handling.Documentation and Examples
Tooltipcomponent to demonstrate usage and configuration.