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

Add custom extractor for sorting for objects. #358

Merged
merged 7 commits into from
Nov 1, 2022
Merged

Conversation

bharathcs
Copy link
Collaborator

Fixes #353

Copy link
Collaborator

@ganhongyao ganhongyao left a comment

Choose a reason for hiding this comment

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

Some design-related comments.

Again, I feel that this is a temporary solution and we should probably move to sorting in the backend if we continue to have to make such code changes in future. Hmm

frontend/components/generic/SimpleTable.tsx Outdated Show resolved Hide resolved
frontend/components/generic/SimpleTable.tsx Outdated Show resolved Hide resolved
@bharathcs
Copy link
Collaborator Author

bharathcs commented Nov 1, 2022

Thanks for the review Hong Yao, I've cleared up the comments you've mentioned. As for the design comment, we resolved this offline in tg, but for leaving a record of our discussion here:

Some design-related comments.

Again, I feel that this is a temporary solution and we should probably move to sorting in the backend if we continue to have to make such code changes in future. Hmm

No point offloading the sorting to the backend if the frontend needs to refetch all of the data over and over again with every change in sort order (by clicking a sort key in the table). Unless we intend to handle filtering and pagination in the backend because of a tremendous amount of data, there's no purpose to optimising this and incurring more network lag.

Plus, all our table data is always <20 mb even if we were to make a million coupons / campaigns so its not like a frontend can't handle sorts.

@ganhongyao ganhongyao merged commit 0875d5d into master Nov 1, 2022
@ganhongyao ganhongyao deleted the bhcs/fix-sort branch November 1, 2022 06:27
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.

Sorting doesn't work for some keys in table.
2 participants