-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: LaunchDevly: Pagination for 'Only show flags with overrides' #426
Conversation
@@ -231,6 +243,7 @@ function Flags({ | |||
setSearchTerm(e.target.value); | |||
setCurrentPage(0); // Reset pagination | |||
}} | |||
value={searchTerm} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously the input of the search box wasn't actually coupled to the display of the search... We were just relying on onChange events.
@@ -247,10 +260,6 @@ function Flags({ | |||
const hasOverride = flagKey in overrides; | |||
const currentValue = hasOverride ? overrideValue : flagValue; | |||
|
|||
if (onlyShowOverrides && !hasOverride) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was causing issues, because we need to not do any filter logic in the display logic for the pagination to work as expected.
return filtered; | ||
}, [flags, searchTerm]); | ||
return flagEntries | ||
.filter((entry) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to be clearer filter entries. I assume we'll add another for tags in the future ;)
Fixes the interaction with pagination when toggling the "only" toggle. Puts the filter ahead of the display logic so that our pagination doesn't get confused why the render is leaving flags out.
This works as expected now.