Skip to content

Conversation

@sachin-maheshwari
Copy link

No description provided.

import _ from 'lodash'
import { axiosInstance as axios } from './requestInterceptor'
import { TC_API_URL } from '../config/constants'
import { TC_API_URL, PROJECT_LIST_CHUNK } from '../config/constants'

Choose a reason for hiding this comment

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

I would prefer PROJECT_LIST_PAGE_SIZE and for now we can use the same constant for card view page size.

Copy link

@vikasrohit vikasrohit left a comment

Choose a reason for hiding this comment

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

@sachin-maheshwari I guess src/api/projects.js is not the only place where we are using magic number 20 for page size. I saw it some other places as well e.g. https://github.com/appirio-tech/connect-app/blob/dev/src/projects/list/components/Projects/ProjectsGridView.jsx#L191 and https://github.com/appirio-tech/connect-app/blob/dev/src/projects/list/components/Projects/ProjectsCardView.jsx#L44. Can you please check other possible areas as well where we might be hard coding the page size?

@sachin-maheshwari
Copy link
Author

@vikasrohit , sure..I'll check and make changes accordingly.

I observed some abnormal behavior of corresponding back-end API, which is giving maximum 20 results . If it is known behavior, need to tune in front-end according - otherwise pagination functionality at front-end will be broken for value PROJECT_LIST_PAGE_SIZE > 20 (as we are using same constant in offset). https://api.topcoder-dev.com/v4/projects/?limit=50&offset=0&fields=id,name,description,members,status,type,actualPrice,estimatedPrice,createdAt,updatedAt,details&sort=updatedAt+desc

@vikasrohit
Copy link

Yes 20 is the max page size that any one can pass to the API. I don't think that has any effect front end except that we should never set page size > 20. So, you can add comment to the newly introduced constant about the same.

@vikasrohit
Copy link

LGTM 👍

@vikasrohit vikasrohit merged commit 4a405d3 into dev Dec 11, 2017
@vikasrohit vikasrohit deleted the tech-debt#1352 branch May 15, 2018 11:21
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.

3 participants