-
Notifications
You must be signed in to change notification settings - Fork 256
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
V8 tracking configuration #2970
Conversation
- allows client-side management of show-view result set navigation
010dac7
to
e1d298f
Compare
@barmintor would you be able to do something with the "squash" commit? |
😆 yes, absolutely |
…search_session - allow configuration of components for applied parameters and item pagination - only build server-side search tracking data if using server-side tracking - extract applied_params html into a component
e1d298f
to
e45aafa
Compare
…t::ServerItemPaginationComponent
e45aafa
to
9bd3ecb
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.
Should Blacklight::SearchContextComponent
be deprecated in this PR?
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.
Looks great! Just noticed two inconstancies..
end | ||
|
||
def search_id | ||
controller.current_search_session&.id |
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.
Is there a reason this is not search_session and you are reaching into the controller?
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 agree this seems like a mistake. The only scenarios I can think of where it might make sense is where @search_session
is nil
or if @search_session
would ever differ from controller.current_search_sesssion
. But neither of those two scenarios makes sense to me either.
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 unchanged from https://github.com/projectblacklight/blacklight/blob/main/app/components/blacklight/search_context_component.rb#L56-L58, but I think y'all are right that it should be changed.
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.
Ok, this is a little complicated: @search_session
is a Hash representation of a search. controller.current_search_session
is an instance of the Search
model. I think we can rely on @search_session['id']
for this value, which should be loaded in a before_action added to controllers when they include Blacklight::Catalog
. But it relies on more assumptions than this implementation.
Again, this was not introduced in this PR. This is the existing behavior in main
of a component that is renamed in this PR.
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 think it might be good to separately ticket this change.
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.
@barmintor I agree with you about a separate ticket vs changing it in this PR. The implications of an instance variables vs a hash representation may be very significant in a way that is not yet clear. I keep wondering if there is a context in which these two different representation are different and it does matter which one is used at this point. That would be bad design but still an open question.
@jcoyne I think the deprecation of |
Hey following up on my reservations from the last committer meeting. I tried to cherry pick this commit onto the 7.33.0 branch to test against my current app. But unfortunately I get the error
on my app when I try to render the show page. Looks like we are overriding a template that specifically references that removed class. But I don't want the fact that I can't test this in my v7 app to block this any further so I'm happy to have it merged and just cross that potential bridge if/when the time comes when we upgrade the app to v8. |
@dkinzer that component is renamed in this PR to |
Co-authored-by: carolyncole <1599081+carolyncole@users.noreply.github.com>
Thanks. I'll try that. |
@barmintor ... with the change you suggest I then get the following error:
|
@dkinzer seems likely to be fixed by the change that @carolyncole requested |
@dkinzer you might also try changing that call from |
@barmintor when I use either methods suggested for the fix I get a new error:
If I try to fix that by calling the private method via a
|
…gating to controller;-helpers
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.
Based on the discussion I'm approving this PR.
This PR pulls the breaking configuration changes out of #2954 without the new features. This would allow us to pull the sessionStorage changes in to Blacklight 8.x in a feature release. This PR:
render?
logic for SearchContextComponent into the componenttrack_search_session
in Blacklight::Configuration from a boolean to a configuration object allowing component configuration