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

Refine Feature Flags and Segments List Sorting #1992

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

zackcl
Copy link
Collaborator

@zackcl zackcl commented Sep 26, 2024

This PR fixes the incorrect sorting of the Segments list after refreshing the page. It also updates the Feature Flags sorting to work just like the Segments and Experiments list tables, ensuring that the previous sorting state remains after a page refresh by storing the sorting states in local storage.

The Feature Flags sorting works fine in general, except that it always defaults to sorting by "Name" in ascending order upon page refresh, even though I can confirm that the previous value is stored in local storage. I would appreciate any ideas on how to fix this. We can also merge this one for now and address that issue later, as it's still an improvement over the current.

@zackcl
Copy link
Collaborator Author

zackcl commented Sep 27, 2024

@ppratikcr7 Just for your info, I realized that featureFlagSortAs and featureFlagSortKey are no longer being stored/used in the code. I think they were stored previously while I was working on this PR (they no longer appear after I deleted them).

Screenshot 2024-09-27 at 10 46 47 PM

I can confirm that the values are correctly stored to and read from the local storage (you can search FeatureFlagLocalStorageKeys.FEATURE_FLAG_SORT_TYPE in the codebase and add logging).

I think setting the default sortKey and sortAs in the feature-flags.reducer.ts might be overriding the values retrieved from the local storage when the page is refreshed.

export const initialState: FeatureFlagState = adapter.getInitialState({
  isLoadingUpsertFeatureFlag: false,
  isLoadingImportFeatureFlag: false,
  isLoadingFeatureFlags: false,
  isLoadingUpdateFeatureFlagStatus: false,
  isLoadingFeatureFlagDetail: false,
  isLoadingFeatureFlagDelete: false,
  isLoadingSelectedFeatureFlag: false,
  isLoadingUpsertPrivateSegmentList: false,
  hasInitialFeatureFlagsDataLoaded: false,
  duplicateKeyFound: false,
  activeDetailsTabIndex: 0,
  skipFlags: 0,
  totalFlags: null,
  searchKey: FLAG_SEARCH_KEY.ALL,
  searchValue: null,
  sortKey: FLAG_SORT_KEY.NAME,
  sortAs: SORT_AS_DIRECTION.ASCENDING,
});

But this is also how it's implemented in the segments.reducer.ts (which works fine), so I'm not sure about this.

Anyway, I'm not sure how to make the Feature Flags root table keep the previous sorting state even after a refresh, just like other pages. I think it's OK to merge this for now unless anyone has specific ideas or suggestions to fix this minor issue.

@zackcl
Copy link
Collaborator Author

zackcl commented Sep 30, 2024

@VivekFitkariwala @ppratikcr7 @Yagnik56 Please feel free to approve this PR.

@ppratikcr7
Copy link
Collaborator

@VivekFitkariwala @ppratikcr7 @Yagnik56 Please feel free to approve this PR.

@zackcl Yeah, it looks good. I also tried to check but you are correct, the code is similar for segments and experiment, couldn't find why on refresh its always ASC order. Maybe we can create a separate ticket for that and tackle that later.

@zackcl zackcl merged commit a2dad5a into dev Oct 1, 2024
14 checks passed
@zackcl zackcl deleted the bugfix/make-sorting-consistent branch October 1, 2024 13:20
@@ -66,7 +93,8 @@ export class LocalStorageService {
};

const state = {
experiments: experimentState, // experiment state,
experiments: experimentState,
featureFlagState: featureFlagState,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@zackcl Change this to featureFlags for local storage to work correctly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That worked, thank you! I created a PR that fixes this: #2005

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.

4 participants