-
Notifications
You must be signed in to change notification settings - Fork 2
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
Full width slide down syllabus chat for the resource drawer #2063
Conversation
@@ -52,7 +52,7 @@ jobs: | |||
POSTHOG_API_HOST: https://app.posthog.com | |||
POSTHOG_PROJECT_ID: ${{ secrets.POSTHOG_PROJECT_ID_PROD }} | |||
POSTHOG_API_KEY: ${{ secrets.POSTHOG_PROJECT_API_KEY_PROD }} | |||
POSTHOG_ENABLE_SESSION_RECORDING: ${{ secrets.POSTHOG_PROJECT_ENABLE_SESSION_RECORDING_PROD }} | |||
POSTHOG_ENABLE_SESSION_RECORDING: ${{ secrets.POSTHOG_ENABLE_SESSION_RECORDING_PROD }} |
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.
Slipped this in - it was just badly named. mitodl/ol-infrastructure#2978 sets the env vars.
useEffect(() => { | ||
const updateHeight = () => { | ||
if (titleSectionRef.current) { | ||
setTitleSectionHeight(titleSectionRef.current.offsetHeight) | ||
} | ||
} | ||
updateHeight() | ||
const resizeObserver = new ResizeObserver(updateHeight) | ||
|
||
if (titleSectionRef.current) { | ||
resizeObserver.observe(titleSectionRef.current) | ||
} | ||
return () => { | ||
resizeObserver.disconnect() | ||
} | ||
}, []) |
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.
Had very much hoped for a CSS only solution for the slide down chat. The issue is that it needs to be in flow to position beneath the title (whose height depends on title length and screen width), though needs to be out of flow and overlaid in stacking context to have a fixed height to the bottom to allow for the slide down transition and to not interfere with the resource content positioning. Inspecting the title section's height to use at its top positioning and enabling/disabling pointer events on the layer seems a reasonable compromise.
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.
Just starting to look at code. Generally worked well, but noticed a few issues:
- Drawer's scrollbar is covered up by title
- no obvious suggestion... Some z-index issue?
- Title close button shifts annoyingly when chat is toggled (see video)
- IMO, we should add
scrollbar-gutter: stable
to the scrolling container so that it reserves space for the scrollbar. (I believe that this is based on the user agent, so if the user agent uses overlay scrolling, no space is reserved.)
- IMO, we should add
- Accessibility: When chat is visible, users can still tab to the hidden content, and vice versa.
- I think that the easiest solution would be to conditionally apply inert to the chat vs regular content.
inert
doesn't have fantastic browser support, but if you're missing it, the page is still pretty functional. - Altenratively, could use transitionend events, I feel like there may be some caveats about when exactly those fire. (Caveats might not be relevant to our case; but e.g., if the transition css is removed so the transition starts but then finishes abruptly, i believe the event never fires.)
- I think that the easiest solution would be to conditionally apply inert to the chat vs regular content.
layout_shifts.mov |
That's not pretty. I don't see that my side - it must be system level scrollbars (are you on Windows/Chrome?). On MacOS/Chrome I get: Screen.Recording.2025-02-20.at.20.30.00.movWill check for ways for me to emulate and try your |
This reverts commit 823d955.
Posting as a backup option if we see cross platform scrollbar issues (@ChristopherChudzicki has a solution, commit) - A workaround is to only scroll the content section when chat is enabled. Although scrollbars at the right of the window don't look good if they're not full height, the horizontal line on the "AskTIM..." opener affords us the option: Screen.Recording.2025-02-20.at.22.26.04.mov |
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.
All the scrolling issues and shifting seem resolved to me, 👍 👍
I'm still seeing one accessibility issue.
- When Chat is open, I can't tab to content ✅ (regular content is inert)
- When Chat is closed, I can still tab into it. ❌
- The chat section needs inert when not open.
Left a few other extremely minor comments.
frontends/main/src/page-components/LearningResourceExpanded/AiChatSyllabusSlideDown.tsx
Outdated
Show resolved
Hide resolved
frontends/main/src/page-components/LearningResourceExpanded/AiChatSyllabusSlideDown.tsx
Outdated
Show resolved
Hide resolved
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.
👍
@@ -49,7 +49,7 @@ const ChatLayer = styled("div")<{ top: number; chatExpanded: boolean }>( | |||
right: 0, | |||
pointerEvents: chatExpanded ? "auto" : "none", | |||
overflow: "hidden", | |||
scrollbarGutter: "stable", | |||
scrollbarGutter: chatExpanded ? "auto" : "stable", |
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.
As discussed in slack, this has the downside that users with classic scrollbars see a small layout shift when expanding/collapsing.
Another way to get the divider to be full-width w/o the layout shift would be to keep this stable, and set the divider's width to 100vw when it's open. Then it will overflow to the edge of the screen. (It needs to be 100% when not open, though, or it overflows into the scrollbar undesirably.)
That said, my suggest seems a little hacky. Up to you.
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 the shift is ok in this case as the scrollbars appear occupying space, and the button shifts to remain centered. As the shift coincides with the scrollbars appearing, it's inoffensive. Let's get feedback though and can revisit if needed.
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/6747
Description (What does it do?)
Screenshots (if appropriate):
How can this be tested?
chatEnabled
totrue
in LearningResourceExpanded.tsx and HeroSearch.tsx.Additional Context