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

Add support for Blacklight hit-highlighting in the full-text field #1030

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

jkeck
Copy link
Contributor

@jkeck jkeck commented Dec 15, 2017

Closes #993

Stolen heavily from #99 👏 @cbeer

TODO

  • Don't render highlight section when there are no highlights

This can be tested locally by indexing objects from the Feigenbaum exhibit. zy575vf8599 is used in the example below.

hit-highlight

}
}

dd.blacklight-full_text_tesimv {

Choose a reason for hiding this comment

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

Avoid qualifying class selectors with an element.

padding-left: 2ch;
}

&.collapsed:after {

Choose a reason for hiding this comment

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

Begin pseudo elements with double colons: ::

@@ -0,0 +1,32 @@
dt.blacklight-full_text_tesimv {

Choose a reason for hiding this comment

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

Avoid qualifying class selectors with an element.

Copy link
Contributor Author

@jkeck jkeck Dec 20, 2017

Choose a reason for hiding this comment

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

We unfortunately have to provide element because the same class is on two elements that we want to treat differently. This was pulled over from the previous PR, and seems like is safe to ignore in this case.

@@ -0,0 +1,21 @@
Blacklight.onLoad(function(){

Choose a reason for hiding this comment

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

'Blacklight' is not defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This addressed by the global above.

@jkeck jkeck force-pushed the hit-highlight-redux branch 3 times, most recently from 675fefd to a84c507 Compare December 19, 2017 23:54
@jkeck jkeck changed the title [WIP] Add support for Blacklight hit-highlighting in the full-text field Add support for Blacklight hit-highlighting in the full-text field Dec 20, 2017
@jkeck jkeck force-pushed the hit-highlight-redux branch 4 times, most recently from 40a73b3 to 6ecbb0f Compare December 20, 2017 00:35
@camillevilla camillevilla self-requested a review December 20, 2017 18:37
@@ -25,6 +25,15 @@ def iiif_drag_n_drop(manifest, width: '40px')
end
end

def render_fulltext_highlight(args)
Copy link
Contributor

Choose a reason for hiding this comment

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

@jkeck what do you think about moving this over to CatalogHelper?

Copy link
Contributor Author

@jkeck jkeck Dec 20, 2017

Choose a reason for hiding this comment

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

Yeah, makes sense. I typically think about the purpose of CatalogHelper as overrides of Blacklight's CatalogHelper, but clearly that's not the pattern here. Just moved this helper over there. 👍

Copy link
Contributor

@camillevilla camillevilla left a comment

Choose a reason for hiding this comment

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

👏 👍

@camillevilla camillevilla merged commit 8181db4 into master Dec 20, 2017
@camillevilla camillevilla deleted the hit-highlight-redux branch December 20, 2017 21:45
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.

3 participants