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

Implement Recent Items Widget #616

Merged
merged 1 commit into from
Sep 25, 2019
Merged

Implement Recent Items Widget #616

merged 1 commit into from
Sep 25, 2019

Conversation

tpendragon
Copy link
Contributor

Closes #597

This PR looks like the following:

Screen Shot 2019-09-24 at 12 27 50 PM

There's an earlier commit that has a more ordered set up, but I personally liked LAE-style cards better. Here's what the earlier set up looked like:

Screen Shot 2019-09-24 at 12 26 40 PM

@tpendragon
Copy link
Contributor Author

tpendragon commented Sep 24, 2019

Note: This still sorts by timestamp, but sorting by recently modified will require a variety of changes through Pomegranate and Figgy which I'll make some issues for.

Created #617, #619, and pulibrary/figgy#3387.

Copy link
Member

@hackartisan hackartisan left a comment

Choose a reason for hiding this comment

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

I really like your screenshot.

I got a bit lost between the templates; not sure if that could be clarified. Otherwise I think one other small change request.

@@ -0,0 +1,4 @@
<% # container for all documents in index view -%>
Copy link
Member

Choose a reason for hiding this comment

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

are these names required by the solr document helper? It seems like it would be nice to have them flipped around aka _recent_items_document and _recent_items_index

Copy link
Member

Choose a reason for hiding this comment

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

Also why can't they live in app/views/spotlight/sir_trevor/blocks/ with the other template?

@@ -0,0 +1,8 @@
<div class="card">
Copy link
Member

@hackartisan hackartisan Sep 25, 2019

Choose a reason for hiding this comment

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

So the single item partial is called index but the collection partial is called document? That is confusing :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. It was how Spotlight had it set up for auto-looking up partials. Since we're using less of that now I can try to avoid it.

@@ -0,0 +1,23 @@
FactoryBot.define do
factory :iiif_resource, class: IIIFResource do
Copy link
Member

Choose a reason for hiding this comment

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

cool, more factories!


url = "https://hydra-dev.princeton.edu/concern/scanned_resources/1r66j1149/manifest"
FactoryBot.create(:iiif_resource, url: url, exhibit: exhibit, manifest_fixture: "1r66j1149-expanded.json", source_metadata_identifier: "12345678", spec: self)
FactoryBot.create(:iiif_resource, url: "https://hydra-dev.princeton.edu/concern/scanned_resources/44558d29f/manifest", exhibit: exhibit, manifest_fixture: "44558d29f.json", spec: self)
Copy link
Member

Choose a reason for hiding this comment

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

It's sort of weird having one url defined above and other defined inside the parens. both lines are sort of long and it would be good to be able to see what's being defined; maybe just make them both multi-line?


before do
# The view spec is separate from the CatalogController, which has this piece
# of configuration in Blacklight. So to get paths to resolve, we need this.
Copy link
Member

Choose a reason for hiding this comment

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

thanks for the comment

# Source metadata identifier to stub out
source_metadata_identifier { nil }
# Reference to the spec to enable the factory to stub out requests.
spec { nil }
Copy link
Member

Choose a reason for hiding this comment

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

define the stub in the factory, never thought of this; seems handy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure I'm a fan yet, but if we're going to try using more factories it seems like something to fiddle with.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling fe1b45f on recent_items_widget into 68c8603 on master.

Copy link
Member

@hackartisan hackartisan left a comment

Choose a reason for hiding this comment

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

Yay for resolving confusion by changing code instead of just adding comments!

@hackartisan hackartisan merged commit 9da1da5 into master Sep 25, 2019
@eliotjordan eliotjordan deleted the recent_items_widget branch July 9, 2020 16:32
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.

Recent Items Widget
3 participants