samvera-labs / ramp

Interactive, IIIF powered audio/video media player React components library. Styleguidist Docs: https://samvera-labs.github.io/ramp/
https://ramp.avalonmediasystem.org/
30 stars 5 forks source link

Add UI styling to indicate structured navigation component is scrollable that works across all browsers #335

Closed elynema closed 7 months ago

elynema commented 8 months ago

Description

Without a scrollbar present at all times, it is not always obvious that the structured navigation is a scrollable element. However, the scrollbars cannot be easily forced in Firefox without adding additional libraries.

An alternate styling solution was proposed in Ramp #287. https://github.com/samvera-labs/ramp/issues/287. We would like to combine both of those solutions.

image.png image.png

We propose including this styling in Ramp, as other users of the structured navigation component may face similar challenges.

Done Looks Like

elynema commented 8 months ago

Height of the container div is set in ramp, but can be overridden with CSS in implementation. The height of the container determines whether it becomes scrollable, so a prop is not needed to eliminate scrolling behavior.

elynema commented 8 months ago

Need a CSS fix b/c a white bar is being added to the top of the structured nav section, regardless of scroll.

image.png
cjcolvar commented 8 months ago

Do we also need to add the same message/fading on the top of the structured nav so users know there is more when scrolling up? For example, a user may click on a link to open a specific section other than the first and it might not be visible that there is more above that section.

elynema commented 8 months ago

@masaball @joncameron It looks like this may be causing issues with Firefox on Android...I can't scroll down below the structured navigation to get to the tabs. Is that low priority enough for it just to be another issue that's not a priority for 7.7?

masaball commented 8 months ago

@cjcolvar Since the structured navigation is numbered, I think having a "Scroll up" message is not really needed. But maybe the visible items simply not starting at 1 would be too subtle an indicator that there is more content above?

@elynema I was not able to replicate an inability to scroll down to the metadata component just now in a quick Firefox test on a couple phones on browserstack so, unless Jon can replicate, I would lean towards making a follow up ticket for the next release/when there's some free cycles to look into it more deeply.

elynema commented 8 months ago

@masaball Attaching a video of what I saw in Firefox on Samsung in Browserstack. I just keep scrolling down the entire time. Perhaps there is a difference in the way you are scrolling and I am scrolling.

Private Zenhub Video

masaball commented 8 months ago

I think there probably was a difference in our scrolling approaches. Were you using your mouse's scroll wheel to scroll or touchpad gesture equivalent? I was able to replicate the behavior in that video using both a scroll wheel and two finger touchpad scrolling.

When I tested earlier, I was clicking and dragging like how you would hold your finger to the screen and move it to move the screen around. Using that method, I do not see any jumpiness or weird behavior.

I would assume what we are seeing with the scroll wheel behavior is glitchiness with how browserstack is trying to translate the mouse events into touch events on the phone. Would be good to test on a physical device to confirm.

elynema commented 8 months ago

@masaball Yes, I was using the scroll wheel on my mouse to scroll. I think you are correct that there could be glitchiness here. I don't have a physical Android device I can test on (I haven't checked Chrome on Android either).

As far as implementing a scroll up like Chris suggested, I believe the only use case would be if someone landed in the middle of a large item via a section URL. I don't discount we might want to consider that, but it's probably not necessary to resolve before this release. I don't think I would like having the fade and scroll message at both top and bottom when in the middle of a longer structured navigation section.

@joncameron Are you ok with that suggestion? Should we bring this to QA for full testing?

masaball commented 8 months ago

Just tested on my phone (Samsung Galaxy S22) in both Chrome and Firefox. Scrolling behavior was working as expected in both.

elynema commented 7 months ago

Functionality is working as expected across browsers and mobile devices.

@masaball After testing this out, we'd like to remove the fade and just use the 'scroll to see more' message.

joncameron commented 7 months ago

Fade has been removed, everything works as expected.