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

Deprecate render_document_partials. #2546

Merged
merged 1 commit into from
Dec 9, 2021
Merged

Deprecate render_document_partials. #2546

merged 1 commit into from
Dec 9, 2021

Conversation

jcoyne
Copy link
Member

@jcoyne jcoyne commented Nov 19, 2021

This was only used by the atom partial renderer. We can replace this with the document component

Fixes #2537

@cbeer
Copy link
Member

cbeer commented Nov 19, 2021

It's also used quite a bit in overrides:

https://github.com/search?q=render_document_partials&type=code

As a kindness to upgraders, maybe it's worth keeping around in 8.x with a deprecation warning?

👍 to removing the atom xml dependency on it, though

@jcoyne jcoyne changed the title Remove render_document_partials Deprecate render_document_partials. Nov 19, 2021
@jcoyne jcoyne force-pushed the remove-document-partials branch 2 times, most recently from 2250c97 to 41653d2 Compare November 19, 2021 21:31
@jcoyne
Copy link
Member Author

jcoyne commented Nov 19, 2021

@cbeer sounds good, I've switched it to a deprecation.

@@ -22,6 +25,7 @@ def render_document_partials(doc, partials = [], locals = {})
render_document_partial(doc, action_name, locals)
end, "\n")
end
deprecation_deprecate render_document_partials: 'Render the document_component instead'
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean? If I get this error I'm not sure what to do. Can we expand this deprecation error? Or if I just put in render_document_component instead of render_document_partials it'll be good?

Copy link
Member Author

@jcoyne jcoyne Nov 30, 2021

Choose a reason for hiding this comment

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

that would be a rather long message to stick here:

Replace this call with:
"document_component = blacklight_config.view_config(:atom).summary_component
render document_component.new(presenter: document_presenter(document), component: :div, show: true)" instead.

This assumes that the current context has a document_presenter and blacklight_config helpers and that :atom is the current config context.

Copy link
Member

Choose a reason for hiding this comment

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

I might be in the minority here, but I'm 👍 to that long message. Changing it to "Replace this call with a document component render, e.g: allthat" might be nice to be more clear?

Copy link
Member

@tpendragon tpendragon left a comment

Choose a reason for hiding this comment

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

I'm removing my block, I'm not going to be able to act on this for a while and I don't want to hold you back.

@bess bess merged commit 79c77e7 into main Dec 9, 2021
@bess bess deleted the remove-document-partials branch December 9, 2021 17:24
jcoyne added a commit that referenced this pull request Dec 9, 2021
cbeer pushed a commit that referenced this pull request Jul 31, 2022
cbeer pushed a commit that referenced this pull request Jul 31, 2022
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 getting rid of render_document_partials
4 participants