-
Notifications
You must be signed in to change notification settings - Fork 15
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
Hubble Stats - Pool Overview #1431
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if we could implement the responsive breakpoints with appropriate margins that were added later on last week in Figma in this issue? Or if you think we should address responsiveness in a separate issue, let me know what works best.
Here's the link: https://www.figma.com/file/KWLQWPgzbMMIj0p5dylcgB/%F0%9F%93%88-Shielded-Pool-Stats?type=design&node-id=1298%3A203096&mode=dev
Looks good so far! Amazing job on the glass effects!! @vutuanlinh2k2
@monaiuu, I think we should create a separate issue to address responsiveness; at least I find it more convenient for me if we choose that way 😅 You can assign that issue to me! |
{isDarkMode ? ( | ||
<ShieldedAssetDark width={40} height={48} /> | ||
) : ( | ||
<ShieldedAssetLight width={40} height={48} /> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use TailwindCSS classes dark:hidden block
and hidden dark:block
to manage component visibility based on theme, rather than React state. This can transform the component into a server component, reducing client-side JavaScript.
<ShieldedAssetLight width={40} height={48} /> | ||
)} | ||
<Typography variant="h5" fw="bold"> | ||
webbParachain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should parameterize this value instead of hardcoding it here.
variant="body1" | ||
className="text-mono-140 dark:text-mono-40" | ||
> | ||
{shortenHex('0x17488912174388', 4)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should parameterize this value instead of hardcoding it here.
</a> | ||
</div> | ||
|
||
<PoolTypeChip type="single" name="Single Asset" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should parameterize this as the component prop as well. Use single
as the default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, the current state of this hook could be improved. I believe we should refactor it to read and synchronize the theme value from local storage. Moreover, the hook should also be able to support both client-side and server-side rendering. would you be so kind to help me create an issue for this? @vutuanlinh2k2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue for this is created here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments.
✅ Deploy Preview for webb hubble statistic is ready! Thanks for the contribution @vutuanlinh2k2
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for webb hubble statistic is ready! Thanks for the contribution @vutuanlinh2k2
To edit notification comments on pull requests, go to your Netlify site settings. |
}; | ||
|
||
const PoolOverviewContainer = () => { | ||
const [poolOverviewData, setPoolOverviewData] = useState<PoolOverviewType>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need the useState
hook here @vutuanlinh2k2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remove this hook and make it as the server component, when we integrate API into this component, we can utilize the async component from React 18 to fetch and render the data without the useState
hook here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment, otherwise LG 👍
Could you kindly ensure that all conflicts are resolved thoughtfully before proceeding with the merge of this PR? Thank you. |
✅ Deploy Preview for webb hubble statistic is ready! Thanks for the contribution @vutuanlinh2k2
To edit notification comments on pull requests, go to your Netlify site settings. |
Summary of changes
webb-ui-kit
Proposed area of change
Put an
x
in the boxes that apply.apps/bridge-dapp
apps/hubble-stats
apps/stats-dapp
apps/webbsite
apps/faucet
apps/tangle-website
libs/webb-ui-components
Reference issue to close (if applicable)
Specify any issues that can be closed from these changes (e.g. Closes #233).
Screen Recording (with dummy data)
If possible provide a screen recording of proposed change.
Screen.Recording.2023-07-15.at.14.37.21.mov
Code Checklist
Please be sure to add .stories documentation if any additions are made to
libs/webb-ui-components
.