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 bug: popover broke when clicking same record in different channels. #3649

Conversation

demiankatz
Copy link
Member

@demiankatz demiankatz commented May 9, 2024

This PR adds some additional data to channel items to help the popover code differentiate between instances when the same item appears in multiple channels. This fixes a bug discussed in #2934.

@demiankatz demiankatz added this to the 9.1.2 milestone May 9, 2024
@demiankatz
Copy link
Member Author

@sturkel89, you should be able to reproduce the problem I described in this comment on the release-9.1 branch. This PR should fix the problem. Do you mind confirming, and also playing with this branch a little bit to see if you can find any other ways to break channels? If this is good, we should get it merged all the way through to #2934 so we can resume work on that.

Copy link
Contributor

@sturkel89 sturkel89 left a comment

Choose a reason for hiding this comment

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

Short story: your bug fix works as intended, and I could not break Channels in the branch!

More detail:

I was able to reproduce exactly what’s described in the comment re: the release-9.1 branch. Very weird behavior!

In the test branch, I found that the problem did not appear at all. I checked all three themes, in both desktop and mobile. I could click on the same item in multiple channels, and each time I went back and forth the popover would function correctly. All the navigation to the record or to other channels worked well, and the other features such as “show items as search results” worked well too.


Critiques: Channels on mobile

These critiques apply to release-9.1 as well as to test.

Popovers are too narrow
On mobile, the popover is a bit too narrow, and some of the text in the second column can’t be seen. The font is very small, so further shrinking on mobile would probably not be good. Is there any other way to fit this content to the screen?
image

Right arrows are cut off
Also, the arrow on the right is hard to see (especially in sandal and bootstrap3)"

Sandal example:
image

However, the tiny bit that’s visible IS clickable and functional. In any case, the arrow is not that important to most touchscreen users because it’s possible to swipe through the items in a channel on mobile; it’s not necessary to click the arrow.

@demiankatz demiankatz merged commit 19da0f0 into vufind-org:release-9.1 May 10, 2024
7 checks passed
@demiankatz demiankatz deleted the release-9.1-channel-popover-same-id-fix branch May 10, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants