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

799 admin campaign list loading #816

Merged
merged 11 commits into from
Sep 10, 2018
Merged

Conversation

shakalee14
Copy link
Contributor

For #799 - changes initial page load for campaign list to be 25 and then gives options to reveal 10,25,50 and all.

@shakalee14 shakalee14 force-pushed the 799-admin-campaign-list-loading branch from 9351504 to 6d5b5fc Compare August 28, 2018 15:00
@schuyler1d
Copy link
Collaborator

Some notes/thoughts.

  • Not sure we have this in our graphQL/Apollo version, but fwiw, there is a pagination model built in that we can/should use if it IS available: https://www.apollographql.com/docs/react/features/pagination.html (but only if it's not too hard)
  • Assuming we're going to keep this, I would suggest that if we do pagination that we do this with offset/limit args -- i.e. both -- not just limit. We might want to use those args, assuming we don't do fetchMore with whatever its args
  • rather than adding it to the end of all those branches, maybe just have at the very end if (listSize) {query = query.limit(listSize)}

@shakalee14
Copy link
Contributor Author

@schuyler1d so is the suggestion to use fetchMore and implement pagination on this page - which then would need offset?

@shakalee14
Copy link
Contributor Author

shakalee14 commented Aug 28, 2018

@schuyler1d doing some research, it looks like we're running a version that has the initial implementation of fetchMore but people have found some issues while using it - reading this thread on an issue created in apollo-client, a known bug is that the "updateQuery method passed to ObservableQuery.fetchMore is receiving the original query variables instead of the new variables that it used to fetch more data". I'm not sure if that will be a problem for us.

@schuyler1d
Copy link
Collaborator

so I'm ++ to skipping the whole fetchMore thing -- just wanted to bring it up.

@shakalee14 shakalee14 force-pushed the 799-admin-campaign-list-loading branch from 9798ad5 to f6dc928 Compare August 29, 2018 19:00
@shakalee14 shakalee14 added S-ready for stage-main (qa) Status (ADMINS ONLY): PR label for those ready to be added for stage: Approved, tests, etc S-testing-on-stage Status (ADMINS ONLY): PR label for presence on a staging instance labels Sep 5, 2018
Copy link
Collaborator

@schuyler1d schuyler1d left a comment

Choose a reason for hiding this comment

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

style nits. no more approval necessary

@@ -3,6 +3,8 @@ import { Campaign, JobRequest, r } from '../models'

export function buildCampaignQuery(queryParam, organizationId, campaignsFilter, addFromClause = true) {
let query = queryParam
const resultSize = (campaignsFilter.listSize ? campaignsFilter.listSize : 0)
const pageSize = (campaignsFilter.pageSize ? campaignsFilter.pageSize : 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's default pagesize to something other than 0 -- maybe 10?
If pageSize is offset' -- then maybe rename to pageOffset` -- 'pagesize' sounds like it would mean the same thing as listsize/resultsize

@@ -17,6 +19,12 @@ export function buildCampaignQuery(queryParam, organizationId, campaignsFilter,
if ('campaignId' in campaignsFilter) {
query = query.where('campaign.id', parseInt(campaignsFilter.campaignId, 10))
}
if (resultSize && !pageSize) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's have cleaner conditionals

if (resultSize) {
   query = query.limit(resultSize)
}
if (pageSize) {
   query = query.offset(pageSize)
}

This was referenced Sep 5, 2018
@shakalee14 shakalee14 merged commit f6dc928 into main Sep 10, 2018
@shakalee14 shakalee14 deleted the 799-admin-campaign-list-loading branch March 26, 2019 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ready for stage-main (qa) Status (ADMINS ONLY): PR label for those ready to be added for stage: Approved, tests, etc S-testing-on-stage Status (ADMINS ONLY): PR label for presence on a staging instance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants