-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/admin facet per set #280
base: master
Are you sure you want to change the base?
Conversation
begin | ||
facet_item = find_item(params[:id]) | ||
if facet_item.visible | ||
facet_item.visible = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this block of code could be extracted out into its own method possibly.
def set_visibility(visibility)
facet_item.visible = visibility
facet_item.save
flash[:success] = t('admin.facets.field_item_added')
end
@@ -35,8 +35,44 @@ def destroy | |||
redirect_to admin_facets_path | |||
end | |||
|
|||
def remove_item | |||
begin | |||
set_visibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need less ambiguity - either the action (and private method) should have the word "toggle", or set_visibility
should take a parameter and set visibility to that value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should add - if we go with "toggle_item", we'd use it to replace both "add_item" and "remove_item"
I think I have a better understanding of this code now. I think the code is confusing as-is, because we've got multiple checks on I think this code needs to be in its own admin area where an admin can very easily and immediately see all the sets and decide which are shown and which are hidden. That should make the code a lot simpler in addition to making the workflow more clear. |
As a follow-up, looking for |
@jechols Thanks for taking a closer look at this code. I agree that it's a good idea to move it to it's own admin space. I was also concerned about topTerms being the only ones available for configuration, so I will take a closer review at how solr works and find a better way to retrieve all sets instead of depending on topTerms. |
@luisgreg99 From the demo in the Sprint Review, it appears the developed feature was more to edit the Set facet to then turn Sets on and off in the Set facet. I don't believe that is the feature desired, and we should have all worked to make this clearer in the ticket. Rather, I believe it was more that we wanted configurable facets (turning them on/off, changing label, etc.) extended so it's possible to have a different configuration for each Set. So Set 1 (Braceros), would have Facet 1, 2 3 and 5 activated, where Set 2 (ChinaVine) might have Facet 2, 4, 5 and 6 activated. So for each Set, you can pick and choose what config options you want on the facets. If that's the case, I don't think we need a separate Admin Panel for this, but rather extend the Configurable Facets view. The default view might be you choose the settings for the Homepage, but then you could select from the Sets, maybe a dropdown, and that would let you choose another set of facet configuration, that is independent of the others. If I'm off base or others have different interpretations, please chime in. |
Thank you for the clarification @wickr. When you mention the idea about extending the "Configurable Facets" view, are you referring to expanding the view rendered at |
@luisgreg99 Yes, that is the view I was referring to. Expanding "Currently Configured Facets" to maybe default to the Homepage, but then you can select a Set to get a new/different group, so "Currently Configured Facets for Gerald Williams Photograph Collection" or similar. |
fixes #216