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

Hubble Stats Breadcrumbs #1470

Merged
merged 5 commits into from
Jul 31, 2023
Merged

Hubble Stats Breadcrumbs #1470

merged 5 commits into from
Jul 31, 2023

Conversation

devpavan04
Copy link
Member

Summary of changes

  • Adds Breadcrumbs to Header component in hubble-stats dapp.

Proposed area of change

  • 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)

Screen Recording

CleanShot.2023-07-28.at.09.33.38.mp4

@devpavan04 devpavan04 added the hubble-stats Issues specific to hubble-stats label Jul 28, 2023
@devpavan04 devpavan04 self-assigned this Jul 28, 2023
@devpavan04 devpavan04 added the needs review 👓 Pull request needs a review label Jul 28, 2023
@github-actions
Copy link

Deploy Preview for webb hubble statistic is ready! Thanks for the contribution @devpavan04

Name Link
🔨 Latest commit cb2e82d4ff94fd9f3161327e0696f80cf5f70cd4
🔍 Latest deploy log https://app.netlify.com/sites/hubble-stats/deploys/64c3f08f76ac97270d11405d
😎 Deploy Preview https://64c3f08f76ac97270d11405d--hubble-stats.netlify.app

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@AtelyPham AtelyPham left a comment

Choose a reason for hiding this comment

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

Left some comments.

Comment on lines 88 to 116
const pathname = usePathname();

const breadCrumbs = useMemo(() => {
const parts = pathname.split('/');
const activeItem = parts[parts.length - 1];

const breadCrumbItems: Breadcrumb[] = [
{
label: 'Hubble Overview',
isLast: activeItem !== '' ? false : true,
icon: (
<ContrastLine className={activeItem !== '' ? 'fill-mono-120' : ''} />
),
href: '/',
},
];

if (activeItem !== '') {
breadCrumbItems.push({
label: activeItem,
isLast: true,
icon: <CoinIcon />,
href: '',
});
}

return breadCrumbItems;
}, [pathname]);

Copy link
Member

Choose a reason for hiding this comment

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

This component should be the "server component" (or async component to fetch TVL and volume), passed fetched data into the Header component to render. Move this logic to the Header component, extract it into a React hook, and use it inside the Header client component.

Copy link
Member

Choose a reason for hiding this comment

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

Also, consider moving static variables (items and footer in this case) outside of the component.

Comment on lines 35 to 37
return (
<Chip
color="grey"
Copy link
Member

Choose a reason for hiding this comment

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

The default props should be defined in the component (isLast here), instead of using BreadcrumbsItem.defaultProps = BreadcrumbsItemsDefaultProps; as you did. You can read more about this here.

value={volume}
isLoading={volume ? false : true}
value={volumeValue}
isLoading={volumeValue ? false : true}
Copy link
Contributor

Choose a reason for hiding this comment

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

isLoading should be applied only when we are loading the tvl and volume value, not when the tvl and volume are undefined. Maybe you can change the value prop here to "-" if those values are undefined.

Copy link
Contributor

@vutuanlinh2k2 vutuanlinh2k2 left a comment

Choose a reason for hiding this comment

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

Left a comment

@AtelyPham AtelyPham added needs changes 🔧 PR has requested changes or conflicts that need to be addressed and removed needs review 👓 Pull request needs a review labels Jul 31, 2023
@devpavan04 devpavan04 marked this pull request as draft July 31, 2023 18:34
@devpavan04 devpavan04 added wip 🚧 Work in-progress and removed needs changes 🔧 PR has requested changes or conflicts that need to be addressed labels Jul 31, 2023
@devpavan04 devpavan04 marked this pull request as ready for review July 31, 2023 19:46
@devpavan04 devpavan04 added needs review 👓 Pull request needs a review and removed wip 🚧 Work in-progress labels Jul 31, 2023
@github-actions
Copy link

Deploy Preview for webb hubble statistic is ready! Thanks for the contribution @devpavan04

Name Link
🔨 Latest commit 552ffa8aa8c58e1b8ca8107c0e7afa6c6c91b35f
🔍 Latest deploy log https://app.netlify.com/sites/hubble-stats/deploys/64c8125963a36d033652d82a
😎 Deploy Preview https://64c8125963a36d033652d82a--hubble-stats.netlify.app

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@AtelyPham AtelyPham left a 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 👍

@@ -1,13 +1,15 @@
'use client';
Copy link
Member

Choose a reason for hiding this comment

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

You can remove 'use client' here.

@devpavan04 devpavan04 merged commit 15ca596 into develop Jul 31, 2023
8 checks passed
@devpavan04 devpavan04 deleted the PS/hubble-stats-breadcrumbs branch July 31, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hubble-stats Issues specific to hubble-stats needs review 👓 Pull request needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TASK] Implement breadcrumbs component hubble stats page
3 participants