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

Added dynamic icons to filters in tools page[Closes #1260] #1309

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

bhat-shubham
Copy link
Contributor

What kind of change does this PR introduce?
This PR Closes Issue #1260 , adding dynamic and meaningful icons to each and every Filter on the Tools Page.

Issue Number:

Screenshots/videos:
Before-
image
After-
image

If relevant, did you update the documentation?
NA

Summary
Adds dynamic icons to the filters in Tools Page.

Does this PR introduce a breaking change?
NO

NA

@bhat-shubham bhat-shubham requested a review from a team as a code owner January 14, 2025 07:14
Copy link

github-actions bot commented Jan 14, 2025

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
website ✅ Ready (View Log) Visit Preview a111362

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (27ef6c9) to head (a111362).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1309   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          387       396    +9     
  Branches       103       106    +3     
=========================================
+ Hits           387       396    +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@benjagm benjagm requested a review from DarhkVoyd January 19, 2025 12:08
Copy link
Collaborator

@benjagm benjagm left a comment

Choose a reason for hiding this comment

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

Can you please revert the changes in the community submodule?

Screenshot 2025-01-19 at 13 09 21

@bhat-shubham
Copy link
Contributor Author

Can you please revert the changes in the community submodule?

Screenshot 2025-01-19 at 13 09 21

Sure Sir! I'll revert the changes.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please revert the changes in the community submodule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, i have reverted The Changes.

Copy link
Member

Choose a reason for hiding this comment

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

How are these changes relevant to the linked issue for Tools Page?

{ hidden: !isDropdownOpen },
'ml-0 mt-0 overflow-hidden transition-all duration-500 ease-in-out',
{
'max-h-0 opacity-0 invisible': !isDropdownOpen,
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this make it more complicated?

Copy link
Member

@DhairyaMajmudar DhairyaMajmudar left a comment

Choose a reason for hiding this comment

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

@bhat-shubham your CI checks are failing pls. fix them, also remove changes from submodule folders

@bhat-shubham
Copy link
Contributor Author

@DhairyaMajmudar I have reverted the changes in the submodule as well as fixed the failing checks.

@bhat-shubham
Copy link
Contributor Author

hey @benjagm @DarhkVoyd I have made the changes you asked for and need suggestion for the one if it needs to be done that way.
Please Review the PR and let me know.

Copy link
Collaborator

@benjagm benjagm left a comment

Choose a reason for hiding this comment

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

LGTM

@benjagm benjagm requested review from DarhkVoyd and removed request for DarhkVoyd March 5, 2025 22:42
Copy link
Member

@DarhkVoyd DarhkVoyd left a comment

Choose a reason for hiding this comment

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

LGTM!

@bhat-shubham
Copy link
Contributor Author

hey @DarhkVoyd, thank you for approving the changes however, you previously mentioned about the object being redundant and how i could use the filters obj instead.
I have that approach ready as well.
Do Let Me Know if you would like to review it as well.

@DarhkVoyd
Copy link
Member

DarhkVoyd commented Mar 7, 2025

hey @DarhkVoyd, thank you for approving the changes however, you previously mentioned about the object being redundant and how i could use the filters obj instead. I have that approach ready as well. Do Let Me Know if you would like to review it as well.

Hey, that's not a big deal. I just suggested instead of separately managing the filterIcons object with filters as keys, we could have just stored them with the existing filters: ex:

const filters = [
    { label: 'Language', accessorKey: 'languages', icon: LanguageIcon },
    ...
];

That would keep the filters data all together and we can just access them during iteration. The filters data as single source of truth. This is fine too. That is just what I would have preferred but that's just my preference.

@bhat-shubham
Copy link
Contributor Author

Got it! @DarhkVoyd , I had mentioned this in a previous review as well, asking if that was the approach you preferred. And it's the same, I won’t be pushing that commit as per your guidance.

Copy link
Contributor Author

@bhat-shubham bhat-shubham left a comment

Choose a reason for hiding this comment

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

Marked as not necessary since the changes are approved. Closing the review now.

Copy link
Member

@DhairyaMajmudar DhairyaMajmudar left a comment

Choose a reason for hiding this comment

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

Thanks for PR, just a minor comment, rest we're almost done.

Co-authored-by: Dhairya Majmudar <[email protected]>
Copy link
Member

@DhairyaMajmudar DhairyaMajmudar left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@DhairyaMajmudar DhairyaMajmudar added the ready-to-merge PR that already has two approvals. label Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge PR that already has two approvals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug: All Filters Show The Same "Filter Icon" In The Tools Page
4 participants