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

Move sub-partials called by show_page_image to helper methods instead #724

Merged
merged 6 commits into from Jul 21, 2017

Conversation

Projects
None yet
2 participants
@jrochkind
Contributor

jrochkind commented Jul 20, 2017

This actually seemed to have a pretty huge impact on performance for
many-membered works. @hackmastera curious if you can confirm/replicate this finding.

I think it ends up not actually THAT much less legible by the end.

With these additional perf PR's to riiif branch, I think there's a chance riiif branch may actually become FASTER than master branch at present!

jrochkind added some commits Jul 19, 2017

inline rights_statement in _chf_download_menu
This did have significant perf impact.

Turning it into a helper instead of a partial had the same perf impact, if we wanted to keep DRY.

But I think this was a false DRY in the first place, it really IS a
different thing, the code is actually cleaning with it being separate,
with it's completely own CSS class, trying to override the CSS
for the 'large' rights statement didn't make a lot of sense or make
the CSS easier to deal with. (The CSS here is still kind of a mess,
but only trying to fix what's relevant for the perf improvement here.
writing maintainable CSS is not something I'm great at yet.)

Note that all the actual heavy-lifting is done by calls to
the presenters _anyway_ (which then call out to CHF::RightsStatement
in turn).  There was just no good reason for the additional layer
of indirection/DRY in the first place, this is an all around
code improvement even aside from perf I think.

This makes me consider turning the OTHER two partials into helper
methods though, to see how that effects perf.
change _chf_download_menu partial to helper
This gives us significantly improved performance in show page when this is being called
lots of times, once for each member. Not entirely sure why, but it does. Rails.
change _show_page_image_file_set_actions partial to helper
for performance. Makes a big difference when called once-per-member on a show page with many members

hackmastera added some commits Jul 21, 2017

Merge branch 'riiif' into member_partials_to_helpers
Conflicts:
	app/assets/stylesheets/local/show.scss
	app/views/curation_concerns/base/_chf_download_menu.html.erb
	app/views/curation_concerns/base/_show_page_image_file_set_actions.html.erb
@hackmastera

This comment has been minimized.

Show comment
Hide comment
@hackmastera

hackmastera Jul 21, 2017

Contributor

I resolved conflicts; left the CSS for rights-statement-inline outside of .work-show; I think that is what was intended as an end state.

Contributor

hackmastera commented Jul 21, 2017

I resolved conflicts; left the CSS for rights-statement-inline outside of .work-show; I think that is what was intended as an end state.

@hackmastera hackmastera merged commit ea0bd50 into riiif Jul 21, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hackmastera hackmastera deleted the member_partials_to_helpers branch Jul 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment