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

Handle pagination in recompose #76

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Handle pagination in recompose #76

wants to merge 14 commits into from

Conversation

felire
Copy link

@felire felire commented Jan 15, 2020

Pagination Actions

You will have to write actions with the following params (only if you want to build a paginationAction, if you don't want that, write actions like always):

  • paginationAction (boolean)
  • reducerName (The name of the reducer you are going to handle this) (use only if paginationAction is true)
  • refresh (Param that probably you receive in your action, you are going to set it to false when you want to have next pages. By default is setted to true)
  • successSelector (This last param have to transform the response and return the following object)
{
  list,
  meta: {
  totalPages,
  currentPage,
  totalItems // This last item is not necessary but maybe you will need it for something specific.
  }
}

Your service will receive an object with the nextPage prop.

Example of using:

//reducer.js

const stateDescription = {
  tickets: null
};

const initialState = completeState(stateDescription);

const reducerDescription = {
  paginationActions: [actions.GET_TICKETS]
};

//actions.js

const formatPagination = response => ({
  list: response.data?.data,
  meta: {
    totalPages: response.data?.meta?.total_pages,
    currentPage: response.data?.meta?.current_page,
    totalItems: response.data?.meta?.total
  }
});

export const actionCreators = {
  getTickets: (newPagination = true) => ({
    type: actions.GET_TICKETS,
    target: ticketsTarget,
    paginationAction: true,
    refresh: newPagination,
    reducerName: "tickets",
    service: TicketService.getTickets,
    successSelector: response => formatPagination(response)
  })
};

//service.js
const getTickets = ({ nextPage }) => api.get(`/tickets?page=${nextPage}`);

If you want to send more info to your service, your payload will have to return an object, not a single value. (Only if you are using paginationActions)

gif paginatt

@@ -0,0 +1,5013 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this file because we use npm

[`${action.target}TotalItems`]: Number(selector(action).meta.totalItems)
})
})
: mergeState(state, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can call onSuccess directly here

? mergeState(state, {
[`${action.target}Loading`]: false,
[`${action.target}Error`]: null,
[`${action.target}`]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[`${action.target}`]:
[action.target]:

Number(selector(action).meta.currentPage) === 1
? selector(action).list
: state[action.target].concat(selector(action).list),
[`${action.target}TotalPages`]: Number(selector(action).meta.totalPages),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to modify completeState as well, in order to add all these new targets

postBehavior(dispatch, response);
if (determination(response)) {
if (
(paginationAction && paginationNotHasFinished && determination(response)) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm I don't understand this part

target: ticketsTarget,
paginationAction: true,
refresh: newPagination,
reducerName: "tickets",
Copy link
Contributor

Choose a reason for hiding this comment

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

Having to specify the reducer you are in doesn't look good to me.

@marianozicavo
Copy link
Contributor

marianozicavo commented Mar 18, 2021

Hola @felire !
Está por salir la version 3.0 de redux-recompose y estamos revisando el estado los PRs abiertos.


Me parece muy copada la idea. Me hace acordar un poco a (esto)[https://react-query.tanstack.com/guides/paginated-queries].
Me entraron dudas viendolo:

  • esto seria solo para separar la metadata del paginado de la data de la pagina o esta haciendo algo mas?
  • a lo mejor se pueden usar los customCompleters de completeState, completeTypes y completeReducer para no tener que introducir lógica a la basethunkaction.
  • habria que mover la documentacion a la nueva carpeta de recipes poniendo algunos ejemplos y si es posible una demo de como se usa

De todas formas voy a pedirle a los otros chicos de react-native que lo miren para que den una opinión, ya que ellos son los que la usan en el día a día.


La forma de poner "al día" este PR si querés hacerlo es

  • cambiar la base branch del PR por la branch 3.0
  • solucionar conflictos y comentarios

meta: {
totalPages,
currentPage,
totalItems // This last item is not necessary but maybe you will need it for something specific.
Copy link
Contributor

Choose a reason for hiding this comment

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

Needed indentation

@@ -1,5 +1,11 @@
import mergeInjections from '../mergeInjections';

const checkPaginationNotHasFinished = (state, pageSelector) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This name is a little bit confusing. Would you like to name it using the positive case ?

@felire
Copy link
Author

felire commented Mar 22, 2021

@marianozicavo I won't have time to complete this the following weeks and months. If anyone from wolox want to complete feel free fo fork my branch and work from there.

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