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

Permit configurable replacement of search tracking with client session storage #2954

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

barmintor
Copy link
Contributor

@barmintor barmintor commented Jan 3, 2023

fixes #2558

Implement separate client-side and server-side components for tracking search session, displaying appliedParams block in show view, and displaying result pagination from show view

Configuration

The track_session_storage configuration changes in this PR from a true/false flag to either false or a configuration object that responds to the keys storage, applied_params_component, item_pagination_component

The default value for this configuration in this PR is:

storage: "server",
applied_params_component:  Blacklight::SearchContext::ServerAppliedParamsComponent, # introduced in this PR
item_pagination_component: Blacklight::SearchContextComponent

This configuration reproduces the legacy enabled behavior; setting the configuration to false is also supported.

The new configuration available is:

storage: "client",
applied_params_component:  Blacklight::SearchContext::ClientAppliedParamsComponent, # default when client
item_pagination_component: Blacklight::SearchContext:: ClientItemPaginationComponent # default when client

Index Behavior

In the storage: "client" configuration, clicking on a search result stores the current search (query string) in sessionStorage, and submits a form that GETs the show view with a counter parameter (without POSTing to the track URL). The page links information (ie, previous/next and result set size/offset display) is added dynamically with data fetched from the current search controller in the show view. This dynamically generated form is not used if the item is being right-clicked or context-clicked (for example, when opening in a new tab).

Show Behavior

In the storage: "client" configuration, a show view that has a URL param for counter will fetch JSON data from a new controller action called page_links that returns previous/next information based on the counter and the currently stored search. If no search is available in the sessionStorage or no data is returned, the scaffolding for search context is removed.

@barmintor barmintor force-pushed the issue-2558_replaceTrackWithClientSessionStorage branch from 3f9cac9 to c097179 Compare January 3, 2023 23:03
app/javascript/blacklight/search_context.js Outdated Show resolved Hide resolved
app/javascript/blacklight/search_context.js Outdated Show resolved Hide resolved
app/javascript/blacklight/search_context.js Outdated Show resolved Hide resolved
})
})
}
Blacklight.onLoad(function(){
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to use onLoad here or can this function be called by doSearchContextBehavior? I think it's confusing to depend on both approaches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doSearchContextBehavior adds an event listener for clicks, which doesn't inspect the DOM (yet). The "page links" behavior in the show view depends on DOM content. I'm trying to refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, pushed a refactor up. I think that I have to either piggyback on Blacklight.onLoad 's event detection, or I would have to reimplement it. I at least relocated the event handler attachment and the onload addition to doSearchContextBehavior.

app/javascript/blacklight/search_context.js Outdated Show resolved Hide resolved
app/javascript/blacklight/search_context.js Outdated Show resolved Hide resolved
app/javascript/blacklight/search_context.js Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

It's important to document the implications of this PR. Where?

@cbeer
Copy link
Member

cbeer commented Jan 18, 2023

I think it'd be good to split the search history tracking from search context pagination. Spotlight, for example, has a pretty significant dependency on search history, but I think it'd be great to get this better client-side context in play too.

I don't think that's necessarily a blocker to merging this, but would be a nice compatibility thing for downstream users.

@barmintor
Copy link
Contributor Author

I updated the PR description to be more explicit about the changes here.

@barmintor
Copy link
Contributor Author

@cbeer I think it would be good to redesign saving a search as an explicit user action? Have a button on the index view if you are logged in to save the search, rather than passively logging searches.

@jcoyne
Copy link
Member

jcoyne commented Feb 2, 2023

@barmintor This has a conflict and needs to be rebased (and re-build the javascript).

@barmintor barmintor marked this pull request as draft February 2, 2023 14:57
@barmintor barmintor added this to the 8.x milestone Feb 2, 2023
@barmintor
Copy link
Contributor Author

@jcoyne assuming we are pursuing the breaking-API PR for 8.0, I'll need to fix this PR up afterwards. I'm converting to draft while I work on #2970

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.

Consider replacing track_url/session mechanism for previous/next docs with window.sessionStorage
4 participants