-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
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. |
There was a problem hiding this 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 -%> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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 :(
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
9887ac1
to
602a9a8
Compare
602a9a8
to
fe1b45f
Compare
There was a problem hiding this 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!
Closes #597
This PR looks like the following:
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: