-
Notifications
You must be signed in to change notification settings - Fork 7
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
Swap order of contents and sidebar #2786
base: main
Are you sure you want to change the base?
Conversation
@taylor-steve I believe this was a fix in blacklight? |
@dnoneill That seems likely, but I don't recall. Do you have any more specifics? We should add that context to the ticket. Did we intend to get rid of the Spotlight specific page layout in Spotlight? |
@@ -0,0 +1,20 @@ | |||
<%# Override Spotlight layout for accessibility: https://github.com/sul-dlss/exhibits/issues/2466 %> | |||
<%# TODO: Spotlight v5 changes this layout %> |
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.
Can we upgrade to Spotlight 5 first? This seems to be putting obstacles in our way for future maintenance.
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.
Most likely? I don't think this is very urgent. Anyway, it's a 4-character change that doesn't impact production usage so not particularly scary as far as obstacles go.
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.
It's not so much that it's a 4 character change. It's adding a new file that we need to remove and test at some later date.
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.
What file would need to be removed? This file would remain.
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.
Doesn't this override what is already provided by spotlight? If you override stuff from upstream, you can drift from the upstream, making maintenance more complicated.
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 see. There is no current structural change in this layout between Spotlight v4 and v5. Exhibits would need this specific override either way. The alternative would be to swap the order in Spotlight.
The order swap decision was contentious, so I didn't know if we were going to impose that for all Spotlight users by default (I think that's what @dnoneill is getting at, that maybe we already decided to do that).
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.
"The order swap decision was contentious"
Can you provide more information about this? I'm wondering if we can spend more time building consensus in the community rather than diverging without having tried to work this out.
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.
Did you read the issue and the linked work within?
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.
Yes, and I didn't see anything contentious in there.
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.
It seems like we should fix this in Blacklight(projectblacklight/blacklight#3252) as it affects all Blacklight projects, not just exhibits.
There isn't a spotlight specific layout? I am confused. |
Closes #2466.
Swaps the order of content and sidebar so that the facets precede content in the tab order.
Note if you test with
annotate_rendered_view_with_filenames
enabled, that counts as content forcontent_for?
so there will be an empty sidebar.