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

feat: wire up search and filter on Team page #4265

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

RODO94
Copy link
Contributor

@RODO94 RODO94 commented Feb 6, 2025

What does this PR do?

This PR follows on from the SearchBox and Filter component PRs. Each component would filter flows and up until this point, they would setFilteredFlows. The issue we run into now is when a user wants to Search and Filter.

To get round this and make it clear what does what. Each component will still search/filter flows but they will set their own array searchedFlows || filteredFlows - this will then be filtered in the useEffect and set a new state variable called matchedFlows

My thinking is that:

  • If you search first
    • searchedFlows will change
    • filteredFlows will be equal to flows
    • searchedFlows is filtered based on whether they exist in filteredFlows
    • The return will be set as matchedFlows
  • if you filter first
    • filteredFlows will change
    • searchedFlows will be equal to flows
    • searchedFlows will be based on whether it exists in filteredFlows
    • The return will be set as matchedFlows

To Test

Use different combinations of searching and filtering, doing so in different orders.

It is best to create your own target filter and toggle it offline or online and just filtering by status. Filtering by multiple criteria shouldn't affect this work.

@RODO94 RODO94 changed the base branch from main to ian/team-page-rebuild February 6, 2025 13:07
@@ -223,6 +235,7 @@ const Team: React.FC = () => {
);
setFlows(sortedFlows);
setFilteredFlows(sortedFlows);
setSearchedFlows(sortedFlows);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't need to setMatchedFlows since this will happen when these two state variables are set

const [searchedFlows, setSearchedFlows] = useState<FlowSummary[] | null>(
null,
);
const [matchingFlows, setMatchingflows] = useState<FlowSummary[] | null>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going through a few iterations, this seemed the easiest to read, while functionally working as expected.

@RODO94 RODO94 marked this pull request as ready for review February 6, 2025 13:16
@RODO94 RODO94 requested a review from a team February 6, 2025 13:40
Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

This looks good and correct - one note on naming here!

Not tested as there's no pizza, I can do so on the feature branch though.

You can configure the GHA to build pizzas on PRs going into ian/team-page-rebuild not just main so that we can test these 👍 I think this is worth doing!

);

useEffect(() => {
const diffFlows =
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an intersection we're checking for, not a difference.

An intersection is an item which is in both lists, a difference would be items in one list but not the other.

@DafyddLlyr
Copy link
Contributor

Tested locally and working as expected 👌

@RODO94 RODO94 merged commit 438f7c7 into ian/team-page-rebuild Feb 6, 2025
3 checks passed
@RODO94 RODO94 deleted the rory/filter-search-wiring branch February 6, 2025 16:53
Copy link

github-actions bot commented Feb 6, 2025

Removed vultr server and associated DNS entries

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.

2 participants