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

fix current logic error in displaying/hiding feature/unfeature buttons #3776

Merged
merged 1 commit into from
Jun 10, 2019
Merged

fix current logic error in displaying/hiding feature/unfeature buttons #3776

merged 1 commit into from
Jun 10, 2019

Conversation

yf508
Copy link
Contributor

@yf508 yf508 commented May 16, 2019

Fixes #3169

The current implementation of showing/hiding feature/unfeature buttons seems to be incorrect.

e.g. for feature button:

<%= link_to "Feature", hyrax.featured_work_path(presenter, format: :json),
data: { behavior: 'feature' },
class: presenter.display_unfeature_link? ? 'btn btn-default collapse' : 'btn btn-default' %>

It says, "if display_unfeature_link returns true, then hide feature button, OTHERWISE, show feature button". While display_unfeature_link returns false, it doesn't necessarily mean feature button needs to be shown. For example, when 5 works have been featured already, both feature/unfeature buttons should be hidden for new objects. Therefore, to make it simple and working:

<%= link_to t('.feature'), hyrax.featured_work_path(presenter, format: :json),
data: { behavior: 'feature' },
class: presenter.display_feature_link? ? 'btn btn-default' : 'btn btn-default collapse' %>

@samvera/hyrax-code-reviewers

@cjcolvar cjcolvar self-requested a review June 5, 2019 16:24
@no-reply
Copy link
Contributor

no-reply commented Jun 5, 2019

This looks good to me.

Added a Check CLA label.

@cjcolvar
Copy link
Member

cjcolvar commented Jun 5, 2019

@yf508 Thanks for the pull request! In order for us to merge this code, we'll need to have signed Contributor License Agreements on file for you as an individual and your employer. (If you work for University of York then it looks like we already have the cCLA.) You can read more about the Samvera community's CLA processes here:
https://wiki.duraspace.org/display/samvera/Samvera+Community+Intellectual+Property+Licensing+and+Ownership

Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

Requires iCLA before merging.

@yf508
Copy link
Contributor Author

yf508 commented Jun 6, 2019

I have sent the iCLA via email.

@cjcolvar
Copy link
Member

👍 Looks like it has been received. Thanks!

Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

👍

@cjcolvar cjcolvar merged commit f58a248 into samvera:master Jun 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature/unfeature buttons don't work when there are five works already featured
3 participants