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

[Dashboard] feat: replace all teams filter in remaining pages #3111

Open
wants to merge 35 commits into
base: feat/enhanced-dashboard
Choose a base branch
from

Conversation

bassgeta
Copy link
Contributor

Description

Replace the old breadcrumb navigation with the new filter component!
Figma link
image

Testing

Basically go to the following pages and make sure that the filter still works and that it displays nicely

  1. Activity feed
  2. Balances
  3. Incoming funds
  4. Teams
  5. Agreements
  6. Members
  7. Permissions

And do note there's one layout bug present but it's being fixed elsewhere
image

When the screen gets wide, the filter is stuck with the content, but the title stays on the lefthand side.

Diffs

Changes 🏗

  • Added the TeamFilter component to the 7 pages above

Deletions ⚰️

  • BreadcrumbsCardSelect component
  • useTeamsBreadcrumbs hook

Resolves #2860

@bassgeta bassgeta requested review from a team as code owners September 17, 2024 14:18
@bassgeta bassgeta self-assigned this Sep 17, 2024
@bassgeta bassgeta force-pushed the feat/2860-replace-teams-navigation branch from 612433e to 963336c Compare September 18, 2024 08:44
@mmioana mmioana self-requested a review September 18, 2024 09:00
Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Smashed another one @bassgeta 💪 👏

Tested and can confirm the new filters are present on the mentioned pages
Screenshot 2024-09-18 at 11 21 12
Screenshot 2024-09-18 at 11 21 21
Screenshot 2024-09-18 at 11 21 35
Screenshot 2024-09-18 at 11 21 39
Screenshot 2024-09-18 at 11 21 45
Screenshot 2024-09-18 at 11 21 51
Screenshot 2024-09-18 at 11 21 58
Screenshot 2024-09-18 at 11 22 03

I know you didn't touch these pages, but can confirm the domain filters is not present on the Extensions or Profile pages
Screenshot 2024-09-18 at 11 22 10
Screenshot 2024-09-18 at 11 22 20

After leaving the refactoring comments for consistency, I'm wondering if it wouldn't make sense to create a new component and just wrap the pages needing the domain filter. What do you think @bassgeta? 😁

const ContentWithDomainFilter: FC<PropsWithChildren> = ({ children }) => {

  return (
    <div className="flex flex-col gap-8">
      <TeamFilter />
      {children}
    </div>
  )
}

Comment on lines 20 to 26
<div className="flex w-full flex-col gap-4 sm:gap-6">
<div className="mb-2">
<TeamFilter />
</div>
<WidgetBoxList items={widgets} />
<div>
<FiltersContextProvider>
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, would suggest to wrap the previous content as you did on the previous pages

<div className="flex flex-col gap-8">
      <TeamFilter />
     ...previous page content here
</div>

@@ -54,6 +49,9 @@ const AgreementsPage: FC = () => {

return (
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well

<div className="flex flex-col gap-8">
      <TeamFilter />
     ...previous page content here
</div>

Comment on lines 102 to 103
<div className="flex flex-col gap-8">
<TeamFilter />
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well

<div className="flex flex-col gap-8">
      <TeamFilter />
     ...previous page content here
</div>

Comment on lines 38 to 41
<div className="flex w-full flex-col gap-6">
<div className="mb-2">
<TeamFilter />
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well

<div className="flex flex-col gap-8">
      <TeamFilter />
     ...previous page content here
</div>

@bassgeta bassgeta force-pushed the feat/2860-replace-teams-navigation branch from 6b8a1c6 to 163fd91 Compare September 19, 2024 10:06
Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Thank you for implementing the suggested changes @bassgeta 💪 💯

Screenshot 2024-09-19 at 13 40 33
Screenshot 2024-09-19 at 13 40 41
Screenshot 2024-09-19 at 13 40 44
Screenshot 2024-09-19 at 13 40 46
Screenshot 2024-09-19 at 13 40 50
Screenshot 2024-09-19 at 13 40 53
Screenshot 2024-09-19 at 13 40 55
Screenshot 2024-09-19 at 13 40 58

Let's get this one merged 🎉

Copy link
Contributor

@davecreaser davecreaser left a comment

Choose a reason for hiding this comment

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

Nice job once again @bassgeta!

It's nice that now the breadcrumbs can just be breadcrumbs 😆 It makes it much simpler! Also agree with the approach of the reusable wrapper component, it's very nice.

It looks like you need to rebase, but I checked the code by just looking at your actual commits and it all looks good. Also tested all the pages you mentioned (including mobile) and everything is as expected! Let's gooo.

Screenshot 2024-09-19 at 17 12 37 Screenshot 2024-09-19 at 17 12 53 Screenshot 2024-09-19 at 17 13 07 Screenshot 2024-09-19 at 17 13 18 Screenshot 2024-09-19 at 17 13 30 Screenshot 2024-09-19 at 17 13 41 Screenshot 2024-09-19 at 17 13 53

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants