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

Adds persistence and reloading of datatable pagination params #2112

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

pavgra
Copy link
Contributor

@pavgra pavgra commented Jan 23, 2020

fixes #760

@pavgra pavgra requested review from anthonysena and vlbe January 23, 2020 15:25
@anthonysena anthonysena added this to the V2.8.0 - Backlog milestone Jan 28, 2020
@anthonysena anthonysena self-assigned this Jan 28, 2020
Copy link
Collaborator

@anthonysena anthonysena left a comment

Choose a reason for hiding this comment

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

@pavgra just testing this out and it seems like what you've built here works well for the data table component itself. That said, it looks like perhaps the other parts of the application need to be more aware of the state of the data table. Take for example the following workflow:

  • Navigate to the cohort definition list via the left-hand menu
  • Filter the list using the search which results in a URL like: http://localhost:81/atlas/#/cohortdefinitions?dtSearch_-1258431457=ehden&dtPage_-1258431457=0
  • Open a cohort definition from that list
  • Close the cohort definition

I'd expect that closing the cohort definition would preserve the query string parameters required to filter the data table contents but that seems to get lost in the process. Could you take a look or let me know if you need any further details? Thanks!

@pavgra
Copy link
Contributor Author

pavgra commented Feb 13, 2020

@anthonysena , I do not think that this case can be implemented in a straightforward and clean way:

  • after you navigated to a specific cohort definition, you may go over tabs back and forward, go into vocabulary search to attach concepts, etc - so, it is impossible to assign a browser's back button behavior to the close button
  • therefore, we would need to store pagination parameters of the previous page:
    • either we can attach them to cohort definition URL - that would be /cohortdefinition/111?searchParamOfTableFromPrevPage=... - but that would make links really ugly and the logic very non-trivial
    • or we could put the params into sessionStorage; however, that would lead to the different behavior of the same page for different users, i.e. you wouldn't be able to share a link and expect the app to behave in the same way (since it would depend on your local data)

@anthonysena
Copy link
Collaborator

I'd like to confirm with @gklebanov around the functional requirements for #760 before we merge this into master.

@anthonysena anthonysena marked this pull request as draft May 12, 2020 13:07
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.

when cohort, concept set is opened and then closed, ATLAS resets the last page that was active
3 participants