[To Main] DESENG-632: Add engagement content tabs #2545
Conversation
met-web/src/utils/index.ts
Outdated
| @@ -81,3 +81,9 @@ export const isDarkColor = (color: string, sensitivity = 0.5) => { | |||
| const luminance = (0.299 * r + 0.587 * g + 0.114 * b) / 255; | |||
| return luminance < sensitivity; | |||
| }; | |||
|
|
|||
| export function combinePromises<T>(promises: { [K in keyof T]: Promise<T[K]> }): Promise<T> { | |||
There was a problem hiding this comment.
| export function combinePromises<T>(promises: { [K in keyof T]: Promise<T[K]> }): Promise<T> { | |
| export function combinePromisesAsObject<T>(promises: { [K in keyof T]: Promise<T[K]> }): Promise<T> { |
When I see the name combinePromises I think "why not just use Promise.all?" I'm a fan of longer variable/function names that provide more description, which I think could be something to consider here. Especially since we don't use doc blocks in this code base
| resizeObserver.disconnect(); | ||
| }; | ||
| } else { | ||
| setTimeout(attachFade, 100); // try again in 100ms |
There was a problem hiding this comment.
Could you help me understand the timeout for attaching the fade? I'm guessing this would run when tabListRef no longer has a current ref but wouldn't the whole component be unmounted by then?
There was a problem hiding this comment.
I'm attaching the fade to a child of the ref (can't find a way to get a ref for the scroller directly) so the timeout is a workaround to let me wait until the ref is loaded before attaching. Note that attachFade is scheduling itself here, so this can be read as "if there is nothing to attach to, try this again in 100 ms"
There was a problem hiding this comment.
We discussed offline and Nat may try useCallback instead of useRef
| const checkFade = () => { | ||
| if (!tabListRef.current) return; | ||
| const scroller = tabListRef.current.getElementsByClassName('MuiTabs-scroller')[0]; | ||
| const scrollPosition = scroller.scrollLeft; // distance from left edge |
There was a problem hiding this comment.
Thanks for the comments so we can follow along more easily!
| const scroller = tabListRef.current.getElementsByClassName('MuiTabs-scroller')[0]; | ||
| const scrollPosition = scroller.scrollLeft; // distance from left edge | ||
| const maxScroll = scroller.scrollWidth - scroller.clientWidth; // distance from right edge | ||
| const fadeMargin = 64; // pixels |
There was a problem hiding this comment.
The fade is a nice idea to make it clear to users that there's more to the tablist if they scroll. As keyboard users tab to the right, will it be clear where their focus is at all times? Will focus ever be obscured by the fade?
There was a problem hiding this comment.
Discussed this offline: keyboard users should be ok
| display: 'flex', | ||
| justifyContent: 'center', | ||
| height: '6px', | ||
| // backgroundColor: 'transparent', |
| height: '100%', | ||
| background: | ||
| 'linear-gradient(to left, rgba(255,255,255,1) 0%, rgba(255,255,255,0.8) 5%, rgba(255,255,255,0.8) 50%, rgba(255,255,255,0) 100%)', | ||
| pointerEvents: 'none', //allow clicking on faded tabs |
There was a problem hiding this comment.
I never knew about this CSS attribute! Neat
| useEffect(attachFade, [tabListRef]); | ||
|
|
||
| return ( | ||
| <section id="content-tabs" aria-label="Engagement content tabs"> |
There was a problem hiding this comment.
Nice aria label for this region! We don't have a heading tag to lean on so I think this is adequately descriptive of the content to follow :)
| import { getWidgets } from 'services/widgetService'; | ||
|
|
||
| const castCustomContentToSummaryContent = (customContent: EngagementCustomContent[]): EngagementSummaryContent[] => { |
There was a problem hiding this comment.
Nice descriptive function name
|
Issue #: 🎟️ DESENG-632
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the met-public license (Apache 2.0).