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

ISLANDORA-2116 adjusted newspaper display to only load first year iss… #157

Open
wants to merge 3 commits into
base: 7.x
Choose a base branch
from

Conversation

wgilling
Copy link

@wgilling wgilling commented Dec 1, 2017

…ues, also added ajax handler to populate for any year clicked

JIRA Ticket: (link)
https://jira.duraspace.org/browse/ISLANDORA-2116

What does this Pull Request do?

This will change the newspaper view so that only the first year's issues are loaded into the web browser -- and all clicks to other years are handled by an AJAX call.

What's new?

This also adds a thumbnail to the view area and automatically expands the loaded fieldsets so that they are not collapsed by default - which may not be desired by some, but our usability study has asked why they were collapsed (I could remove that modification from this pull request).

This would change what gets indexed by search engines for the initial load of the page.

How should this be tested?

A thorough test would be to:

  1. (before merging this code), load a newspaper into the browser that has more than one year of issues and preferably thousands of issues. Inspect the load time for this web page.
  2. Install the updated code here and clear the drupal cache. Repeat step 1 to see how the load time should be much quicker with far fewer requests (only the first year should be loaded into the browser initially).
  3. ensure that each year can be clicked and loaded into the right side of the screen where the issues are displayed.

Additional Notes:

As I do not see any specific documentation relative to the newspaper viewer, there probably isn't any need to change the wiki documentation.

@whikloj
Copy link
Member

whikloj commented Dec 1, 2017

Nice work @bgilling, I'm going to pull this down and play with it. We probably (though I'm not sure) want to allow people to turn on with an admin option.
@mjordan you might want to check this out too.

@whikloj
Copy link
Member

whikloj commented Dec 1, 2017

So this works, but it adds in the thumbnail stuff. I'll see if I can switch it back to the default display and then test it on our dev server and possibly on our backend production server.

@wgilling
Copy link
Author

wgilling commented Dec 1, 2017

Drats -- I can replace this commit with one that does not have the thumbs... gimme 15 mins.

(10 mins later) Ok -- this commit should not contain the thumbs anymore ... nor the change to the collapsable fieldsets.

@whikloj
Copy link
Member

whikloj commented Dec 1, 2017

Thanks @bgilling, that was better there are still some changes in the CSS classes I think as it doesn't look exactly the same.

However, I did some testing and I didn't really see any speed improvement. Perhaps I need to do some tweaking, but for a page with 27,986 issues both the current loading all at once and using Ajax took 186-189 seconds.

There is definitely some processing improvement but the processing time only accounted for 5-6 seconds with the old way.

I think this work is good, but I'm thinking about using this with Solr and faceting the dates so instead of retrieving the entire list of issues you grab the one years list and display all the years.

@wgilling
Copy link
Author

wgilling commented Dec 1, 2017

@whikloj - are all 27,986 issues from the same year? It almost sounds like the drupal cache needs to be cleared. Our load time went from multiple minutes to a second or two.

@whikloj
Copy link
Member

whikloj commented Dec 4, 2017

@bgilling Nope, but we were doing some ingesting so I tried it again today and it was much quicker in general. The difference was approximately 6 seconds for 7.x to approximately 4 seconds for your branch.

But maybe we could get someone else to test this out just in case there is something weird in our system. @mjordan ?

@wgilling wgilling force-pushed the uls_ISLANDORA_2116 branch 2 times, most recently from 40ea27b to 2c3b008 Compare December 14, 2017 21:53
@wgilling
Copy link
Author

Ok - I just addressed the travis coder issues. Was there anything outstanding before this could be merged?

@whikloj
Copy link
Member

whikloj commented Dec 15, 2017

@bgilling probably needs an admin page toggle, so people can continue to get their normal functioning until they enable this.

Just a checkbox on the newspaper solution pack admin page to enable Ajax would be good.

…ues, also added ajax handler to populate for any year clicked
@DiegoPino
Copy link
Contributor

@whikloj you were already on this. Seems like @bgilling addressed the toggle thing. Could you give it a last look? Thanks!

@DiegoPino
Copy link
Contributor

@whikloj ping ping. Let me know if you still have time for this. If not could you help me finding another newspaper user that could test this? Thanks!!

@whikloj
Copy link
Member

whikloj commented Apr 4, 2018

Ok, coming back at this and I notice that it automatically expands the monthly fieldsets and then they can't be collapsed.

Is that expected or it that something in my setup?

@DiegoPino
Copy link
Contributor

@whikloj sorry, don't know... I had issues with collapsing fieldsets in islandora/drupal in the past in bootstrap because of how we at islandora set them up, but can't say for sure here, do you have a sandbox around? If not, no worries, probably already late to get this into the release if there are doubts about missing functionality, code freeze is in like 5 hours or so. But maybe for next one!

@wgilling
Copy link
Author

wgilling commented Apr 4, 2018 via email

@whikloj
Copy link
Member

whikloj commented Apr 5, 2018

@DiegoPino I don't really have a sandbox, but I can probably put this on our development site.

@bgilling I don't think it takes too many issues to make the collapsible fieldsets nice. Our largest paper is a daily so approximately 300 issues on a page.

I implemented an ajax loader for our newspapers and the fieldsets work to expand and contract.

Perhaps you need the ajax_command_html function to get the fieldsets to render correctly.

@whikloj
Copy link
Member

whikloj commented Apr 5, 2018

Forgot to add this, but here is our largest newspaper using the ajax year loader. http://digitalcollections.lib.umanitoba.ca/islandora/object/uofm%3A1243378

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.

5 participants