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 pagination constants #41

Merged
merged 6 commits into from
Oct 31, 2018

Conversation

kwelch
Copy link
Contributor

@kwelch kwelch commented Oct 23, 2018

Adding constants for our pagination methods.

Description

Used internally for a determining how to fetch the next page.

fixes: EB-94542

How Has This Been Tested?

Just constants so I didn't really test anything besides making sure validate passed.

Screenshots (if appropriate):

Checklist:

  • I have read the CONTRIBUTING document.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • I have run yarn validate to ensure that tests, typescript and linting are all in order.

src/constants.ts Outdated
CONTINUATION: 'continuation',
};

export const PAGINATION_MAP = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are currently using this internally, I think we can abstract it a bit and add it here. I think we may need to address some of the terms, but I wanted thoughts before I went too far down this path.

@jonathancreamer-eb
Copy link

@jonathancreamer-eb
Copy link

@kwelch-eb
Copy link
Contributor

I am open to either. I guess my question is how/why would someone need these strings? I think the real power in the other lookup I added.

src/constants.ts Outdated
@@ -0,0 +1,18 @@
export const PAGINATION_TYPE = {
Copy link
Contributor

@benmvp benmvp Oct 23, 2018

Choose a reason for hiding this comment

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

Thinking I'm how this would get used:

import {PAGINATION_TYPE} from 'eventbrite';

const {PAGINATION} = PAGINATION_TYPE;

That seems a bit ugly.

How about:

import {PAGINATION_TYPE} from 'eventbrite';

? Which would mean we should do:

export const PAGINATION_TYPE = 'page';
export const CONTINUATION_TYPE = 'continuation';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am good with that.

src/constants.ts Outdated
@@ -0,0 +1,2 @@
export const PAGINATION = 'page';
Copy link
Contributor

Choose a reason for hiding this comment

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

How about naming them PAGE_KEY & CONTINUATION_KEY? Not sure what I'd expect to get if I import {PAGINATION} from 'eventbrite', but the string 'page' seems surprising.

Copy link
Contributor

@benmvp benmvp left a comment

Choose a reason for hiding this comment

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

The failure in Node 11 is probably due to outdated dependencies. If the upgrade is terrible, let's make a ticket and tackle it later. The nice thing is that now we know.

@kwelch-eb
Copy link
Contributor

@benmvp Updated the travis file to remove it and created #43

@benmvp
Copy link
Contributor

benmvp commented Oct 25, 2018

Nice, sounds good! I dunno how we wanna reconcile Github issues w/ internal Jira. Might need to create a Jira that just links to the Github issue?

@kwelch-eb
Copy link
Contributor

Yeah I thought about that not really sure. It will easily lose visibility being only in GH.

@kwelch-eb kwelch-eb merged commit f89e908 into eventbrite:master Oct 31, 2018
@ebtravis
Copy link
Collaborator

ebtravis commented Nov 6, 2018

🎉 This PR is included in version 1.0.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants