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

Support pagination in ReplicatorDB activity panel #1288

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

popojargo
Copy link
Member

Overview

Add support for pagination in Replicatory activity panel.

Testing recommendations

GitHub issue number

Related Pull Requests

Supersed #1075

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
  • Update rebar.config.script with the correct tag once a new Fauxton release is made

@popojargo popojargo requested review from garrensmith and Antonio-Maranhao and removed request for garrensmith June 9, 2020 04:16
@popojargo popojargo changed the title Feature/replication paginate Support pagination in ReplicatorDB activity panel Jun 9, 2020
@popojargo
Copy link
Member Author

@garrensmith @Antonio-Maranhao Do I have to make some magic for partitioned database?

Copy link
Contributor

@Antonio-Maranhao Antonio-Maranhao left a comment

Choose a reason for hiding this comment

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

I also found a couple of issues:

  • The page crashes if _replicator DB doesn't exist
  • Polling interval loads a different page.
    • E.g. while viewing the 3rd page with 5 docs/page (_find payload: {limit: 6, skip: 10, selector: {_id: {$gte: null}}}), the results where refreshed but the query payload was for the 2nd page instead of the 3rd (i.e. {"limit":6,"skip":5,"selector":{"_id":{"$gte":null}}})

app/addons/replication/reducers.js Show resolved Hide resolved
app/addons/replication/reducers.js Show resolved Hide resolved
popojargo and others added 2 commits June 13, 2020 15:57
- Change statusTime to startTime in table
- Handle _replicatorDB not found
- Fix replication reducer
- Fix polling refresh
@brianewilkins
Copy link
Contributor

In inspected the commit a35a657
and, insofar as I understand it, it all looks good to me.
I do see that the commit contains fixes for the specific faults pointed out by @Antonio-Maranhao at #1288 (review)

I have played with it a bit (I created 110 continuous replications to do so) and did not find anything wrong with it.

So +1 from me, if that counts for anything.

@Antonio-Maranhao
Copy link
Contributor

@popojargo sorry for the delay. I'll make time to review this today or tomorrow.

Copy link
Contributor

@Antonio-Maranhao Antonio-Maranhao left a comment

Choose a reason for hiding this comment

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

Besides my suggestion, just wanted to point out that column sorting and filtering only affect the docs in the current page, which I'm sure it'll mislead users.

Maybe it'd be worth adding a note at the top @popojargo @brianewilkins ?

app/addons/replication/reducers.js Outdated Show resolved Hide resolved
Co-authored-by: Antonio Maranhao <30349380+Antonio-Maranhao@users.noreply.github.com>
@popojargo
Copy link
Member Author

Besides my suggestion, just wanted to point out that column sorting and filtering only affect the docs in the current page, which I'm sure it'll mislead users.

Maybe it'd be worth adding a note at the top @popojargo @brianewilkins ?

Brainstorming here:

  • We could maybe add a tooltip to the sort buttons? That said, that would be a bit repetitive.<
  • Another variant of the tooltip could be to add a tooltip to the right of the action bar. We would only have one tooltip for all sort buttons.
  • We could add a note above the table. That would take some height some something that might not be important?

@Antonio-Maranhao
Copy link
Contributor

@popojargo I'd go with a note above the table. The tooltips are probably too repetitive as you said and less obvious.

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.

None yet

4 participants