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

(I #2638) Feature to Goto a page directly #2639

Merged
merged 99 commits into from Jun 17, 2020

Conversation

antoine2711
Copy link
Member

(Closes #2638)

First implementation. Is functional.

Background-color for each « button » & validation for the page choice of the user: number, > 1 & < last & singular for a one page project.
@wetneb
Copy link
Sponsor Member

wetneb commented May 24, 2020

Thanks a lot! I am thinking that as a user it might be more natural to go to a particular row or record number, since the pagination changes depending on the filters applied and the page size.

@antoine2711
Copy link
Member Author

antoine2711 commented May 24, 2020

Thanks a lot! I am thinking that as a user it might be more natural to go to a particular row or record number, since the pagination changes depending on the filters applied and the page size.

Good question @wetneb. As a programer, I try to stay in the same logic that was there before. With the « page » concept, if you choose 2, and you go back one page with « previous », you end up strait with the first record at the top. I only mimicked the way the « last page » works, which takes into account the hidden rows.

DataTableView.prototype._onClickGotoPage = function(elmt, evt) {
var lastPage = Math.floor((theProject.rowModel.filtered - 1) / this._pageSize);
var promptMessage = "You are currently at page "+ (this._currentPage) +".\n"
if(lastPage > 1) {
promptMessage += "There are "+ (lastPage + 1) +" pages ";
} else {
promptMessage += "There is 1 page ";
}

It really follow thru the current's OR logic, so I fell this way is less disruptive to the user. But I might be wrong.

As with the other features I coded, my aim is to be as sure as possible it won't break. I know I'm not setting new marks in term of UI, but it's decent, simple, and… working. Also, no dirty tricks in the code, it really a lot on existing code/way OR works.

Of course, there could be:

  • go to page…
  • go to record…
  • go to row…

All beside each other. If you think this is the way, I'll code it.

Regards,
Antoine

@wetneb
Copy link
Sponsor Member

wetneb commented May 24, 2020

Yes, the API exposed by the backend is not really suited to go to a specific row at the moment: this would require backend changes.

In terms of UI, if we just do it based on page numbers as proposed in this PR, I think we could potentially have a number input directly in the table header, which would show the current page number: the user could edit it directly and confirm with the Enter key or a small button next to it.

Let's not rush this and ask @ostephens and @lisa761 what they think about it.

@antoine2711
Copy link
Member Author

Yes, the API exposed by the backend is not really suited to go to a specific row at the moment: this would require backend changes.

We could circumvent having to change the API by bringing the user to the page that contains the row or the first row of the record. It wouldn't be the first row of the page, but the user would have what he wants.

In terms of UI, if we just do it based on page numbers as proposed in this PR, I think we could potentially have a number input directly in the table header, which would show the current page number: the user could edit it directly and confirm with the Enter key or a small button next to it.

Yes, in terms of UI, that would be better. I didn't choose that path because it's a little bit more complex to code, and much less robust, even if it's not important or with effects if it breaks.

I also think we should consider this: by giving a small feature asked from a long time, effective even if in a basic form, we give value to the user at a minimal cost. Then, user usually feel entitled to come and comment about that (usually in the form: « thanks for this feature (…) but (…) ;-) ). Then, it's easy to get a feel of our user base, and act accordingly, with a possible incremental change in the next version. It gives a feeling to the user that they are listened to. And, in fact, it's not just a feeling, it's a processus and it's real.

It might not seem like that to a seasoned developer on GitHub, but there is a step learning curve for a user not too tecky to come here and start to interact. It's a bit intimidating also.

Let's not rush this and ask @ostephens and @lisa761 what they think about it.

Of course. Anyway, it's out now, and people can make their own mind and see if they like it! ;-)

Best Regards, @wetneb,
Antoine

P.S. I feel I've now pushed enough features in the last month, and it's now time to pause. But… if I was to continue, the next feature on the path would probably be to let a user change the name of a facet. It's also something that could be done simply, and without too much risks of breaking, and that would help a good bunch of users, I feel. (That is also another feature that would be MUCH easier to code with a GenericFacet JS parent class…)

But, instead, let's go in debug/testing mode for v3.4! ;-)

@wetneb
Copy link
Sponsor Member

wetneb commented May 24, 2020

Yes you have been incredibly productive, thank you very much! And I have the impression that you are tackling issues which are directly motivated by your own experience with the tool - those contributions are extremely valuable since they make a big difference to users. So thank you again!

@lisa761
Copy link
Member

lisa761 commented May 25, 2020

This would be nice to keep if we keep a toggle between infinite scrolling and pagination. It's a useful feature for the users and the code doesn't seem to introduce anything particularly hard to work with for scrolling.

Changed from prompt() to <input type="number">, and visual X out Y.
@thadguidry
Copy link
Member

thadguidry commented May 25, 2020

@antoine2711

  1. Any way for the down arrow in the input box to not rollover into negative integers, but instead 1 would be the lowest? I like the fluidity of being able to hold down the down arrow... letting it rollover back down to 1 at the lowest, releasing my mouse button, and page 1 would show. Right now, it doesn't work that way, if I hold down the mouse on down arrow, it goes past 1 into negative numbers and if I release my mouse button in the negative number range, the current page is selected.

  2. I'd also try increasing the height of the input box to 16 pixels, as long as it doesn't adversely affect display of rows (this allows the up/down arrows to slightly increase for easier clicking)

  3. The input box width of 50px cuts off display of 1 million + rows, so we should probably bump that to 70px width? Or can/should we auto-size that width?

  4. The wording of [#####] of ##### leaves me wondering what the context is of those numbers... we should probably add the term pages after it? [#####] of ##### pages

  5. Allow the focus to remain in the input box (until users click outside of it), so that users can easily use their Arrow keys on keyboard to go forward or back on page numbers. Currently, the input box loses focus the moment that the up or down arrow on my keyboard is pressed and it goes to the next page. More useful to stay focused in input box, so that I can tap my up arrow on keyboard to continually go forward until I click outside of the input box and lose the focus?

Everything else works fantastic and this is a major improvement!

This is how it looks for me on Windows with Firefox:
image

If the user choose below 1, 1 will be displayed, and if the user choose above the max, the max page will be displayed.
Add pages after « of maxValue », calculate the width of <input> based on max value.
@antoine2711
Copy link
Member Author

antoine2711 commented May 25, 2020

Hi @thadguidry,

  1. Any way for the down arrow in the input box to not rollover into negative integers, but instead 1 would be the lowest? I like the fluidity of being able to hold down the down arrow... letting it rollover back down to 1 at the lowest, releasing my mouse button, and page 1 would show. Right now, it doesn't work that way, if I hold down the mouse on down arrow, it goes past 1 into negative numbers and if I release my mouse button in the negative number range, the current page is selected.

Very interesting question. Yesterday, I coded so that if the user go over the max, that it sticks to the max. I coded it yesterday, it's now pushed to GitHub. I now did the same with the lower limit, that is, if you go below 1, it will stick at 1. I think this will solve the issue you are raising here.

That code is now pushed.

  1. I'd also try increasing the height of the input box to 16 pixels, as long as it doesn't adversely affect display of rows (this allows the up/down arrows to slightly increase for easier clicking)

Done.

  1. The input box width of 50px cuts off display of 1 million + rows, so we should probably bump that to 70px width? Or can/should we auto-size that width?

Auto-size is difficult. It could be calculated… There. Did it. 20px + 8px per number.

  1. The wording of [#####] of ##### leaves me wondering what the context is of those numbers... we should probably add the term pages after it? [#####] of ##### pages

Yes indeed. What would still be missing, is the i18n + TEMPLATING ! @wetneb, could you point me to an example of templating? Or a documentation link to implement it? I think I remember you talked about that in another issue.

  1. Allow the focus to remain in the input box (until users click outside of it), so that users can easily use their Arrow keys on keyboard to go forward or back on page numbers. Currently, the input box loses focus the moment that the up or down arrow on my keyboard is pressed and it goes to the next page. More useful to stay focused in input box, so that I can tap my up arrow on keyboard to continually go forward until I click outside of the input box and lose the focus?

Yes, I also think this would be great. I tried to implement it, but failed :-( Any suggestion as to what I should do? Trapping the key down, setting an internal variable to « refocusAfterChange == true » and then, after change, put back the focus?

I'm dealing with internal browser behavior that I would have to modify… That maybe easier to ask than to code…

Everything else works fantastic and this is a major improvement!

Thank you! I'm happy you like it. It's actually not much code for a lot of user benefit! You now have a new version to checkout! The ball is in your court.

Regards,
Antoine

@thadguidry
Copy link
Member

Hmm, negative numbers continue to show instead of stopping on 1.
Tried on both Edge (Chromium) and Firefox, depending on if you increase, release mouse button, and then decrease as shown:

PageInput

Copy link
Member

@thadguidry thadguidry left a comment

Choose a reason for hiding this comment

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

This seems like it would be easier to just use the control attributes themselves?
<input type="number" id="something" name="something" min="1">
REF: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number

Correct min and max for <input>
@antoine2711
Copy link
Member Author

This seems like it would be easier to just use the control attributes themselves?
<input type="number" id="something" name="something" min="1">
REF: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number

I think I did a mistake… I've put min and max as CSS styles… ;-) Silly me… Anyway, it's now fixed.

Regards, A.

Add managment to keep the arrow's key in the CurrentPage <input>.
Fixes for Thad’s KeyDown's « Infinite Paging » & PageSize changes.
Code rehookCurrentPageInput & spacing for PageSize section
@antoine2711
Copy link
Member Author

Ok then this must be something specific to my own setup, it is curious.

I will test it on Windows 10. Is that close to your setup, @wetneb? Also, I changed the way the delay was working, now, it really delay.

Regards,
Antoine

Copy link
Member

@lisa761 lisa761 left a comment

Choose a reason for hiding this comment

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

This is probably because of the merge conflict. If you could undo the last commit, maybe I can take a look at the conflicts again? Because #2719 changed a lot of things with the table.

Fix headerTable to tableHeader (PR OpenRefine#2719)
data-header-table to data-table-header
Remove .data-header-table-container
@wetneb wetneb merged commit d7aaac2 into OpenRefine:master Jun 17, 2020
@antoine2711
Copy link
Member Author

@wetneb: Window 10

GC:
image

FF:
image

This all seams good.

Regards,
Antoine

@wetneb
Copy link
Sponsor Member

wetneb commented Jun 17, 2020

Yeah, not on my Debian, but that's a cosmetic issue that should not be a blocker for @lisa761's work.

@tfmorris
Copy link
Member

@antoine2711 If this:

I've put back « page(s) » in french, let's deal with that some other time.

implies that you don't want to help test French plurals support, I really wish you'd reconsider, since I spent the time fixing it specifically for you.

@@ -537,6 +537,7 @@
"core-views/view": "Aperçu",
"core-views/extend-not-supported": "Ce service de réconciliation de supporte pas l'extension de données. Essayez de supprimer le service et de l'ajouter à nouveau. Si le problème persiste, contactez le fournisseur de service.",
"core-views/to-text": "En texte",
"core-views/goto-page": "$1 de $2 page(s)",
Copy link
Member

Choose a reason for hiding this comment

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

This should be pluralized the same way Spanish and English is.

@antoine2711
Copy link
Member Author

antoine2711 commented Jun 17, 2020

@antoine2711 If this:

I've put back « page(s) » in french, let's deal with that some other time.

implies that you don't want to help test French plurals support, I really wish you'd reconsider, since I spent the time fixing it specifically for you.

Hi @tfmorris, please, when you see one of my action and you don't think it's inline with « OR standard » or your way of seeing things, do NOT assume it's true or it's my intention. You will have a better communications with me if you instead ask my thru a question.

In this particular case, the reason I removed this (after putting IT BACK a few days ago), it's because @wetneb wanted this issue resolve fast for his GSoS.

Also, I didn't want to make « noise » here about this, since it's not directly related. My intentions are to tackle this with PR #2717, where you will soon see me requesting your attention in order to fixed. From there, I will then fix this, also. From what I understand of you, you should like this way of working.

Regards,
Antoine

@antoine2711 antoine2711 deleted the issue-2638-goto-any-page branch April 17, 2022 07:01
wetneb added a commit to wetneb/OpenRefine that referenced this pull request Nov 8, 2022
…ne#2639)"

Go back to simply displaying a range of row numbers, because
that will enable a solution for #33, OpenRefine#570 and OpenRefine#572.
This reverts commit d7aaac2.
wetneb added a commit that referenced this pull request Dec 5, 2022
…#5411)

* Adapt the GridState interface to separate sorting and pagination.

This adds `getRowsBefore` methods, counterpart to the existing pagination
methods, so that we can do efficient and correct pagination, solving #33 and #570

* Revert "(I #2638) Feature to Goto a page directly (#2639)"

Go back to simply displaying a range of row numbers, because
that will enable a solution for #33, #570 and #572.
This reverts commit d7aaac2.

* Do not move back to the first page when applying an action which refreshes the grid.

Instead, the current page is refreshed.
This closes #33, closes #570, closes #572.

* Specify which actions preserve row and record ids

This lets us preserve pagination only when it is guaranteed to show the same
part of the data after the operation.

* Make paging user-editable like before

* Adapt cypress tests
wetneb added a commit that referenced this pull request Oct 10, 2023
…#5411)

* Adapt the GridState interface to separate sorting and pagination.

This adds `getRowsBefore` methods, counterpart to the existing pagination
methods, so that we can do efficient and correct pagination, solving #33 and #570

* Revert "(I #2638) Feature to Goto a page directly (#2639)"

Go back to simply displaying a range of row numbers, because
that will enable a solution for #33, #570 and #572.
This reverts commit d7aaac2.

* Do not move back to the first page when applying an action which refreshes the grid.

Instead, the current page is refreshed.
This closes #33, closes #570, closes #572.

* Specify which actions preserve row and record ids

This lets us preserve pagination only when it is guaranteed to show the same
part of the data after the operation.

* Make paging user-editable like before

* Adapt cypress tests
wetneb added a commit that referenced this pull request Nov 12, 2023
…#5411)

* Adapt the GridState interface to separate sorting and pagination.

This adds `getRowsBefore` methods, counterpart to the existing pagination
methods, so that we can do efficient and correct pagination, solving #33 and #570

* Revert "(I #2638) Feature to Goto a page directly (#2639)"

Go back to simply displaying a range of row numbers, because
that will enable a solution for #33, #570 and #572.
This reverts commit d7aaac2.

* Do not move back to the first page when applying an action which refreshes the grid.

Instead, the current page is refreshed.
This closes #33, closes #570, closes #572.

* Specify which actions preserve row and record ids

This lets us preserve pagination only when it is guaranteed to show the same
part of the data after the operation.

* Make paging user-editable like before

* Adapt cypress tests
wetneb added a commit to wetneb/OpenRefine that referenced this pull request Apr 18, 2024
…f the grid (OpenRefine#5411)

* Adapt the GridState interface to separate sorting and pagination.

This adds `getRowsBefore` methods, counterpart to the existing pagination
methods, so that we can do efficient and correct pagination, solving #33 and OpenRefine#570

* Revert "(I OpenRefine#2638) Feature to Goto a page directly (OpenRefine#2639)"

Go back to simply displaying a range of row numbers, because
that will enable a solution for #33, OpenRefine#570 and OpenRefine#572.
This reverts commit d7aaac2.

* Do not move back to the first page when applying an action which refreshes the grid.

Instead, the current page is refreshed.
This closes #33, closes OpenRefine#570, closes OpenRefine#572.

* Specify which actions preserve row and record ids

This lets us preserve pagination only when it is guaranteed to show the same
part of the data after the operation.

* Make paging user-editable like before

* Adapt cypress tests
wetneb added a commit to wetneb/OpenRefine that referenced this pull request Apr 18, 2024
…OpenRefine#5411)

* Adapt the GridState interface to separate sorting and pagination.

This adds `getRowsBefore` methods, counterpart to the existing pagination
methods, so that we can do efficient and correct pagination, solving #33 and OpenRefine#570

* Revert "(I OpenRefine#2638) Feature to Goto a page directly (OpenRefine#2639)"

Go back to simply displaying a range of row numbers, because
that will enable a solution for #33, OpenRefine#570 and OpenRefine#572.
This reverts commit d7aaac2.

* Do not move back to the first page when applying an action which refreshes the grid.

Instead, the current page is refreshed.
This closes #33, closes OpenRefine#570, closes OpenRefine#572.

* Specify which actions preserve row and record ids

This lets us preserve pagination only when it is guaranteed to show the same
part of the data after the operation.

* Make paging user-editable like before

* Adapt cypress tests
wetneb added a commit to wetneb/OpenRefine that referenced this pull request Apr 18, 2024
…OpenRefine#5411)

* Adapt the GridState interface to separate sorting and pagination.

This adds `getRowsBefore` methods, counterpart to the existing pagination
methods, so that we can do efficient and correct pagination, solving #33 and OpenRefine#570

* Revert "(I OpenRefine#2638) Feature to Goto a page directly (OpenRefine#2639)"

Go back to simply displaying a range of row numbers, because
that will enable a solution for #33, OpenRefine#570 and OpenRefine#572.
This reverts commit d7aaac2.

* Do not move back to the first page when applying an action which refreshes the grid.

Instead, the current page is refreshed.
This closes #33, closes OpenRefine#570, closes OpenRefine#572.

* Specify which actions preserve row and record ids

This lets us preserve pagination only when it is guaranteed to show the same
part of the data after the operation.

* Make paging user-editable like before

* Adapt cypress tests
wetneb added a commit to wetneb/OpenRefine that referenced this pull request Apr 19, 2024
…OpenRefine#5411)

* Adapt the GridState interface to separate sorting and pagination.

This adds `getRowsBefore` methods, counterpart to the existing pagination
methods, so that we can do efficient and correct pagination, solving #33 and OpenRefine#570

* Revert "(I OpenRefine#2638) Feature to Goto a page directly (OpenRefine#2639)"

Go back to simply displaying a range of row numbers, because
that will enable a solution for #33, OpenRefine#570 and OpenRefine#572.
This reverts commit d7aaac2.

* Do not move back to the first page when applying an action which refreshes the grid.

Instead, the current page is refreshed.
This closes #33, closes OpenRefine#570, closes OpenRefine#572.

* Specify which actions preserve row and record ids

This lets us preserve pagination only when it is guaranteed to show the same
part of the data after the operation.

* Make paging user-editable like before

* Adapt cypress tests
wetneb added a commit to wetneb/OpenRefine that referenced this pull request Apr 19, 2024
…OpenRefine#5411)

* Adapt the GridState interface to separate sorting and pagination.

This adds `getRowsBefore` methods, counterpart to the existing pagination
methods, so that we can do efficient and correct pagination, solving #33 and OpenRefine#570

* Revert "(I OpenRefine#2638) Feature to Goto a page directly (OpenRefine#2639)"

Go back to simply displaying a range of row numbers, because
that will enable a solution for #33, OpenRefine#570 and OpenRefine#572.
This reverts commit d7aaac2.

* Do not move back to the first page when applying an action which refreshes the grid.

Instead, the current page is refreshed.
This closes #33, closes OpenRefine#570, closes OpenRefine#572.

* Specify which actions preserve row and record ids

This lets us preserve pagination only when it is guaranteed to show the same
part of the data after the operation.

* Make paging user-editable like before

* Adapt cypress tests
wetneb added a commit to wetneb/OpenRefine that referenced this pull request Apr 24, 2024
…OpenRefine#5411)

* Adapt the GridState interface to separate sorting and pagination.

This adds `getRowsBefore` methods, counterpart to the existing pagination
methods, so that we can do efficient and correct pagination, solving #33 and OpenRefine#570

* Revert "(I OpenRefine#2638) Feature to Goto a page directly (OpenRefine#2639)"

Go back to simply displaying a range of row numbers, because
that will enable a solution for #33, OpenRefine#570 and OpenRefine#572.
This reverts commit d7aaac2.

* Do not move back to the first page when applying an action which refreshes the grid.

Instead, the current page is refreshed.
This closes #33, closes OpenRefine#570, closes OpenRefine#572.

* Specify which actions preserve row and record ids

This lets us preserve pagination only when it is guaranteed to show the same
part of the data after the operation.

* Make paging user-editable like before

* Adapt cypress tests
wetneb added a commit to wetneb/OpenRefine that referenced this pull request May 16, 2024
…OpenRefine#5411)

* Adapt the GridState interface to separate sorting and pagination.

This adds `getRowsBefore` methods, counterpart to the existing pagination
methods, so that we can do efficient and correct pagination, solving #33 and OpenRefine#570

* Revert "(I OpenRefine#2638) Feature to Goto a page directly (OpenRefine#2639)"

Go back to simply displaying a range of row numbers, because
that will enable a solution for #33, OpenRefine#570 and OpenRefine#572.
This reverts commit d7aaac2.

* Do not move back to the first page when applying an action which refreshes the grid.

Instead, the current page is refreshed.
This closes #33, closes OpenRefine#570, closes OpenRefine#572.

* Specify which actions preserve row and record ids

This lets us preserve pagination only when it is guaranteed to show the same
part of the data after the operation.

* Make paging user-editable like before

* Adapt cypress tests
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.

Feature to Goto a page directly