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

Retrieve previous and next members in a work #2418

Merged
merged 14 commits into from
Oct 30, 2023

Conversation

eddierubeiz
Copy link
Contributor

@eddierubeiz eddierubeiz commented Oct 13, 2023

Ref #2322 .

The UI is clunky, but perhaps good enough for a first pass.
I may actually add a second issue for figuring out where to place the links on the page.

Requirements:

  • Should work with a combo of assets and works as members
  • Doesn't break if there are nil or duplicate positions
  • Doesn't break if the positions aren't contiguous integers
  • Should be moderately fast
  • Should work if the asset or work has no parent

@eddierubeiz
Copy link
Contributor Author

Featuring my new friends LAG and LEAD.

@jrochkind
Copy link
Contributor

jrochkind commented Oct 15, 2023

i did not know about lag/lead... intersting!

I would hae just done two queries > $current_pos, LIMIT 1, and < $current_pos LIMIT 1. (with ordering ot break ties consistently etc).

Which I think would have been fine! But maybe using this fancy thing is fine too? pg-specific? Also fine. As long as there's a test that will fail if it breaks in the future!

@eddierubeiz eddierubeiz changed the title [WIP] Retrieve previous and next members in a work Retrieve previous and next members in a work Oct 20, 2023
@eddierubeiz eddierubeiz marked this pull request as ready for review October 20, 2023 16:25
@eddierubeiz
Copy link
Contributor Author

(Did a bit of research - turns out LAG and LEAD are pretty widespread.)

@jrochkind
Copy link
Contributor

Very well organized code, with tests, thanks! I like making the helper class, and calling it in the controller.

Do we really need the names of the previous/next assets on screen? I think not, especially because asset titles aren't very meaningful?

Unless @apinkney0696 has asked for them or wants them, I think simple links « PREVIOUS | NEXT » are best, perhaps greyed-out/not-a-link when you are at first/last? Similar to our bottom of the page search results pagination.

Putting extra text on the page can end up just confusing with too many things for the eye to look at, instead of helpful. There is already a lot on this page!

If you really wanted to, you could make the title a hover popover of some kind, since we do already have it, but I suspect it's not necessary! As a user, do you want it?

@jrochkind
Copy link
Contributor

You could make them bootstrap class: "btn btn-sm" small buttons, but I think text links are actually probably cleaner and preferable here! Our search results pagination links are simple text links.

@eddierubeiz
Copy link
Contributor Author

I'll take all these suggestions - thanks!

@jrochkind
Copy link
Contributor

And actually, you know what, if we don't need to display the title, taht actually saves you some SQL fetches, does it not? You are doing separate fetches to fetch in the records to get title?

There is probably a way to do that all in one fetch... but if you don't need the titles, and I don't think we do, it's not an issue, don't do those extra fetches!

@eddierubeiz
Copy link
Contributor Author

Good call- stay tuned...

@jrochkind
Copy link
Contributor

looking good, thanks

@jrochkind jrochkind merged commit e0315cd into master Oct 30, 2023
1 check passed
@jrochkind jrochkind deleted the admin_asset_previous_next_buttons branch October 30, 2023 19:18
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.

2 participants