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

inline rights_statement in _chf_download_menu #723

Merged
merged 1 commit into from Jul 21, 2017

Conversation

Projects
None yet
2 participants
@jrochkind
Contributor

jrochkind commented Jul 19, 2017

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 cleaner 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 does make me consider turning the OTHER two partials into helper
methods though, to see how that effects perf. I think we're down to perf being rails issues alone no longer sufia/samvera-related. rails is just slow at: 1) partials, and 2) generating URLs via routing. (riiif branch may generate more urls than master or certainly than stock sufia)

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

This comment has been minimized.

Show comment
Hide comment
@hackmastera

hackmastera Jul 21, 2017

Contributor

I'm not sure why you didn't inline the other call to this partial, as well, and remove the partial entirely? Especially since it seems like you're unhappy with the theme parameter.

Contributor

hackmastera commented Jul 21, 2017

I'm not sure why you didn't inline the other call to this partial, as well, and remove the partial entirely? Especially since it seems like you're unhappy with the theme parameter.

@hackmastera hackmastera merged commit 21037d1 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 inline_rights_statement branch Jul 21, 2017

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Jul 23, 2017

Contributor

@hackmastera yes, I think you're right, I just didn't get to thinking about it, was focusing on the perf goal.

I thought I saw commits where you doing that in the github history, but this is merged without it -- I've gotten confused as to what's going on with the various PR's, can you give me an update? :)

Contributor

jrochkind commented Jul 23, 2017

@hackmastera yes, I think you're right, I just didn't get to thinking about it, was focusing on the perf goal.

I thought I saw commits where you doing that in the github history, but this is merged without it -- I've gotten confused as to what's going on with the various PR's, can you give me an update? :)

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