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

GT-1646 Provide several filters for the overall list of tools #702

Merged
merged 12 commits into from
Feb 26, 2024

Conversation

caleballdrin
Copy link
Contributor

Description

https://jira.cru.org/browse/GT-1646
Currently we show all tools and lessons when loading the admin tool. We should add a few filters at the top of the list that would allow us to filter by tool type, system, and hidden tools.

It would also be nice to have an option to sort the list of tools by the default order or alphabetically.

Changes I made

  • Rearranged exisiting buttons
  • Added dropdown buttons for Filter and Sort
  • Used localStorage to hold filter and sort preferences
  • Add filters and sorting when resources are loaded based on the localStorage filters

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

Attention: Patch coverage is 80.39216% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 66.32%. Comparing base (2720ff9) to head (fb7027c).

Files Patch % Lines
...rc/app/components/resources/resources.component.ts 80.39% 6 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #702      +/-   ##
==========================================
+ Coverage   65.89%   66.32%   +0.42%     
==========================================
  Files          59       59              
  Lines        1434     1482      +48     
  Branches      103      115      +12     
==========================================
+ Hits          945      983      +38     
- Misses        475      481       +6     
- Partials       14       18       +4     

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

@CruGlobal CruGlobal deleted a comment from stage-branch-merger bot Feb 20, 2024
@CruGlobal CruGlobal deleted a comment from stage-branch-merger bot Feb 20, 2024
@CruGlobal CruGlobal deleted a comment from stage-branch-merger bot Feb 20, 2024
@CruGlobal CruGlobal deleted a comment from stage-branch-merger bot Feb 20, 2024
@CruGlobal CruGlobal deleted a comment from stage-branch-merger bot Feb 20, 2024
Copy link

@canac canac left a comment

Choose a reason for hiding this comment

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

I'm going to let someone with more familiarity test the functionality, I only reviewed the code itself.


this.resources = this.filters
? resources.filter((r) => {
let visible = true;
Copy link

Choose a reason for hiding this comment

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

Instead of using a visible variable, I would replace each visible = false with return false and then return true at the bottom.

src/app/components/resources/resources.component.ts Outdated Show resolved Hide resolved
src/app/components/resources/resources.component.ts Outdated Show resolved Hide resolved
0,
);

this.resources = this.filters
Copy link

Choose a reason for hiding this comment

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

This logic is kind of complex, so it would be a good candidate for adding tests for. Or at least moving the filtering logic into a helper function and adding tests for the helper.

Copy link
Contributor

Choose a reason for hiding this comment

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

Second this. Make it easier when writing tests

src/app/components/resources/resources.component.ts Outdated Show resolved Hide resolved
src/app/components/resources/resources.component.html Outdated Show resolved Hide resolved
src/app/components/resources/resources.component.html Outdated Show resolved Hide resolved
src/app/components/resources/resources.component.html Outdated Show resolved Hide resolved
src/app/components/resources/resources.component.html Outdated Show resolved Hide resolved
src/app/components/resources/resources.component.html Outdated Show resolved Hide resolved
0,
);

this.resources = this.filters
Copy link
Contributor

Choose a reason for hiding this comment

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

Second this. Make it easier when writing tests

src/app/components/resources/resources.component.ts Outdated Show resolved Hide resolved
src/app/components/resources/resources.component.ts Outdated Show resolved Hide resolved
src/app/components/resources/resources.component.ts Outdated Show resolved Hide resolved
Copy link

Merge conflict attempting to merge this into staging. Please fix manually.

@CruGlobal CruGlobal deleted a comment from stage-branch-merger bot Feb 23, 2024
@CruGlobal CruGlobal deleted a comment from stage-branch-merger bot Feb 23, 2024
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

I love the new changes. One UX piece I think we could improve on is the Sort select.

The user should be able to see that it's sorting by without having to click on it to find out.

Screenshot 2024-02-23 at 9 56 28 AM

This is Walmart's sort by. See how it gives you all the information you need.
Screenshot 2024-02-23 at 9 59 30 AM

package.json Outdated Show resolved Hide resolved
>
Default Order
Tool Order
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you been asked to change this name from the God Tools team? If not, I would set it back to what it was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't asked but I think it makes it more clear, like you mentioned in your earlier comment. I will ask them what they prefer.

Copy link

Merge conflict attempting to merge this into staging. Please fix manually.

@dr-bizz dr-bizz self-requested a review February 23, 2024 16:13
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

The code looks good. This can be moved to QA

@caleballdrin caleballdrin merged commit 5768719 into master Feb 26, 2024
4 checks passed
@caleballdrin caleballdrin deleted the filter-sort branch February 26, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants