indicate stanford-only restriction even after login #1763

jmartin-sul · 2023-11-16T22:02:03Z

~~requires sul-dlss/stacks#1049~~ (merged)

This is the simpler approach to dealing with #1531, from which we can iterate and do something more like the ideal mockup. (per comment on the ticket: "Simpler approach: a persistent banner across the tab (i.e., not collapsible). That is also fine.")

mockup for the simpler approach

TODO:

fix the immediate post-login UI issue where the post-login banner pushes up the menu bar with the hamburger icon that lets you open the companion window (see screenshot).
fix the fresh page load after login issue where the logged in banner pushes down the player so that the controls are cut off.
~~add a working log out link~~ none of the logout link possibilities discussed in standup seemed to work for me, will ticket as separate follow on work
specs? from post-standup discussion, seemed like this maybe isn't worth a feature spec (and like it might be hard to write one)? but maybe worth a view component spec to make sure the banner renders when expected and not when not? there's already a spec to this effect (media_with_companion_windows_component_spec.rb
file a ticket for the logout link (investigate/implement log out link #1902)
file a follow-on ticket for the nicer design version of 1531? (Implement the more complex design for the logged in access restriction message? #1903)

Existing login banner:

New banner indicating successful login, with close button (I tried unsuccessfully to float it to the right, then settled for this because it seemed somewhat defensible to have it the "close" button placed similarly to the Login button before login -- happy to revisit if that's too much rationalizing):

Video after dismissing the post-login message:

jmartin-sul · 2023-11-21T20:48:04Z

pointers for logout link implementation:

example from mirador from alan: https://purl.stanford.edu/bb627py6670
mike says see H2 -- https://stanfordlib.slack.com/archives/C05SVRXGER1/p1700590285579929
- First, configure webauth library with the magic shib URL: https://github.com/sul-dlss/happy-heron/blob/362f154952b856e6d861c38280b0f2906bb07ae2/config/initializers/devise_remote_user.rb#L13
- Next, wire up devise to destroy the session: https://github.com/sul-dlss/happy-heron/blob/362f154952b856e6d861c38280b0f2906bb07ae2/config/routes.rb#L9-L11
- Last, create a link in the UI: https://github.com/sul-dlss/happy-heron/blob/362f154952b856e6d861c38280b0f2906bb07ae2/app/views/layouts/application.html.erb#L76

jmartin-sul · 2023-11-21T22:40:09Z

question for @alundgard:

What should I do about the logged in banner either pushing the player controls under the bottom of the embed iframe in PURL, or pushing the hamburger icon for the companion window above it (which behavior occurs depends on whether the user just logged in, or already logged in before loading the page).

From post-standup discussion, the options seemed like:

shrink the video so that it fits in with the banner without chopping off the controls or laying the banner over the video
try the fancier design from the first mockup, with the unlock icon off to the side, and either minimized or expanded with companion window, overlayed on video (jcoyne observes: downside of this approach is that on iphone size screens or for wide aspect ratio videos, you'll always be covering part of the video)
put a close button on the banner after it's initially shown? but then what to do about presenting a log out button?

alundgard · 2023-11-21T22:58:14Z

I think this makes the most sense: "shrink the video so that it fits in with the banner without chopping off the controls or laying the banner over the video." Player controls should remain visible.
Since we're going with the "simple" persistent banner for now, adding a banner "close/dismiss/X" button after login could be a good compromise design. I'll think more about this possibility, and check with the POs. Thanks!

jmartin-sul · 2023-11-30T07:49:20Z

app/assets/stylesheets/media.scss

-  // First selector is for embargo. Last selector is other auth restrictions
-  .authLinkWrapper + .media-component-body, [data-controller="auth-restriction"]:has(.authLinkWrapper:not([hidden])) + .media-component-body {
-    max-height: calc(100vh - 88px); // Scroll height for metadata when the auth wrapper is displaying
+  [data-controller="auth-restriction"]:not([hidden]) + .media-component-body {


without the selector simplification here and the associated HTML and JS updates (motivation described in more detail the last commit message), i found that the video player wouldn't expand to fill the space left by the dismissed banner. also, the video player would be a bit jumpy on page load, and would be shrunk as if the banner was already displayed, even if the banner had yet to be shown because a response from stacks had yet to be received.

the fix associated with this CSS tweak seems to smooth things out so that the video player consistently shrinks when auth message is displayed, and expands when it's not.

example of the incorrectly unexpanded player before the fix in the last commit (seems to be a problem post-login even on main at the moment):

Does this still look right for an embargo? It looks like that selector has been removed entirely.

ok, after a bunch of uncertainty, i did what i thought was the safest thing to move this PR along without introducing a regression: i put the old selector back. but i made two small tweaks: i flipped the order, because that felt more natural given the order of the DOM elements in media_with_companion_windows_component.erb, and i updated the comment because i'm pretty sure the selector i added back is for citation-only.

per stereotypically wordy slack reply, there's a corner case with two banners that neither the old CSS nor the slightly re-jiggered CSS seem to account for. but i don't think this PR makes that worse, so maybe we can circle back on that in a follow up if need be.

jmartin-sul · 2023-11-30T07:50:54Z

app/assets/stylesheets/media.scss

-  .authLinkWrapper + .media-component-body, [data-controller="auth-restriction"]:has(.authLinkWrapper:not([hidden])) + .media-component-body {
-    max-height: calc(100vh - 88px); // Scroll height for metadata when the auth wrapper is displaying
+  [data-controller="auth-restriction"]:not([hidden]) + .media-component-body {
+    max-height: calc(100vh - 100px); // Scroll height for metadata when the auth wrapper is displaying


if i left it at 88px, the bottom of the control bar was still a bit cut off, e.g.

vs after this fix:

jmartin-sul · 2023-11-30T07:52:25Z

app/assets/stylesheets/media.scss

+      svg {
+        font-size: 0.85rem;
+      }
+    }


this styling was just the result of a bit of tweaking till i got something that i thought looked slightly better than the default. very open to suggestions here.

app/javascript/src/controllers/media_tag_controller.js

app/javascript/controllers/auth_restriction_controller.js

… more accurately reflect current behavior

…re consistent * start with the auth-restriction div hidden when the DOM is initially rendered * un-hide it in the methods that do other DOM manipulation to display auth restriction messages conditionally based on authResponse * hide it if the auth-restriction message is dismissed * simplify the CSS selector for media-component-body sizing, since now it only has to care about whether auth-restriction div is hidden or not * rename a method for accuracy, take advantage of a possible simplification (stimulus auth_restriction_controller.dismissStanfordRestriction becomes .hideLoginPrompt) otherwise, i found that the old CSS was sometimes subtracting from max-height for media-component-body when it should've been left full height, presumably some things in the more complex selector weren't hidden as expected.

jmartin-sul force-pushed the iss1531-display-post-login-stanford-restriction_take2 branch 5 times, most recently from f86c39c to 490472b Compare November 21, 2023 17:56

jmartin-sul mentioned this pull request Nov 21, 2023

send some basic access restriction info along with successful auth check response sul-dlss/stacks#1049

Merged

jmartin-sul force-pushed the iss1531-display-post-login-stanford-restriction_take2 branch 2 times, most recently from e70fbf6 to c71b2da Compare November 30, 2023 02:08

jmartin-sul changed the title ~~[WIP] indicate stanford-only restriction even after login~~ indicate stanford-only restriction even after login Nov 30, 2023

jmartin-sul commented Nov 30, 2023

View reviewed changes

jmartin-sul marked this pull request as ready for review November 30, 2023 08:00

jmartin-sul changed the title ~~indicate stanford-only restriction even after login~~ [requires stacks 1049] indicate stanford-only restriction even after login Nov 30, 2023

jmartin-sul force-pushed the iss1531-display-post-login-stanford-restriction_take2 branch from 2275267 to 10333cc Compare November 30, 2023 21:38

jcoyne reviewed Dec 1, 2023

View reviewed changes

app/javascript/controllers/auth_restriction_controller.js Outdated Show resolved Hide resolved

jmartin-sul force-pushed the iss1531-display-post-login-stanford-restriction_take2 branch 2 times, most recently from 06804d6 to adbe637 Compare December 1, 2023 09:46

jmartin-sul added 7 commits December 1, 2023 14:04

indicate stanford-only restriction even after login

0b55b8d

rename auth_restriction_controller.hideMessage to .hideLoginPrompt to…

4d3b70f

… more accurately reflect current behavior

show stanford restricted message on initial page load when logged in

84be0cb

fix media component display height when auth banner is showing

bd4af20

add button for dismissing post-login stanford restriction message

f4df29d

add back errantly removed CSS selector, update comment for accuracy

827343e

jmartin-sul force-pushed the iss1531-display-post-login-stanford-restriction_take2 branch from adbe637 to 827343e Compare December 1, 2023 22:05

jmartin-sul changed the title ~~[requires stacks 1049] indicate stanford-only restriction even after login~~ indicate stanford-only restriction even after login Dec 2, 2023

This was referenced Dec 2, 2023

investigate/implement log out link #1902

Open

Implement the more complex design for the logged in access restriction message? #1903

Open

jmartin-sul requested a review from jcoyne December 2, 2023 03:30

aaron-collier approved these changes Dec 5, 2023

View reviewed changes

jcoyne merged commit 4a5a354 into main Dec 5, 2023
1 check passed

jcoyne deleted the iss1531-display-post-login-stanford-restriction_take2 branch December 5, 2023 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

indicate stanford-only restriction even after login #1763

indicate stanford-only restriction even after login #1763

jmartin-sul commented Nov 16, 2023 •

edited

Loading

jmartin-sul commented Nov 21, 2023

jmartin-sul commented Nov 21, 2023

alundgard commented Nov 21, 2023

jmartin-sul Nov 30, 2023 •

edited

Loading

jcoyne Dec 1, 2023

jmartin-sul Dec 1, 2023

jmartin-sul Nov 30, 2023

jmartin-sul Nov 30, 2023

indicate stanford-only restriction even after login #1763

indicate stanford-only restriction even after login #1763

Conversation

jmartin-sul commented Nov 16, 2023 • edited Loading

jmartin-sul commented Nov 21, 2023

jmartin-sul commented Nov 21, 2023

alundgard commented Nov 21, 2023

jmartin-sul Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

jcoyne Dec 1, 2023

Choose a reason for hiding this comment

jmartin-sul Dec 1, 2023

Choose a reason for hiding this comment

jmartin-sul Nov 30, 2023

Choose a reason for hiding this comment

jmartin-sul Nov 30, 2023

Choose a reason for hiding this comment

jmartin-sul commented Nov 16, 2023 •

edited

Loading

jmartin-sul Nov 30, 2023 •

edited

Loading