Overlay option added in exercise component and new component IncludeRemoveQuestion created #86
Conversation
…emovequestion created
src/components/Card.tsx
Outdated
| @@ -87,9 +87,6 @@ const StepCardHeader = styled.div` | |||
| button.ox-icon-angle-left, button.ox-icon-angle-right { | |||
| display: none; | |||
| } | |||
| .separator { | |||
There was a problem hiding this comment.
This style was causing an error for lg display
There was a problem hiding this comment.
Yeah this should be fixed in #85
src/components/Card.tsx
Outdated
| @@ -87,9 +87,6 @@ const StepCardHeader = styled.div` | |||
| button.ox-icon-angle-left, button.ox-icon-angle-right { | |||
| display: none; | |||
| } | |||
| .separator { | |||
There was a problem hiding this comment.
Yeah this should be fixed in #85
src/components/Exercise/index.tsx
Outdated
| @@ -195,15 +213,32 @@ export const Exercise = styled(({ | |||
| {...(exerciseIcons ? { exerciseIcons: exerciseIcons } : null)} | |||
| className={props.className} | |||
| > | |||
| <div ref={container}> | |||
| <div | |||
| ref={hoverRef} | |||
There was a problem hiding this comment.
I think these need to be swapped right? So the hover happens anywhere inside the card? I think that also means that the tabindex needs to be placed on the TaskStepCardWithToolbar
There was a problem hiding this comment.
Well actually, while I'm changing this, I see that TaskStepCardWithToolbar covers more than the visible card:
Screen.Recording.2025-01-23.at.11.32.33.AM.mov
There was a problem hiding this comment.
Oh right there's some big margins on that... maybe we add a new wrapper for the toolbar and card body
There was a problem hiding this comment.
@jivey
In order to avoid modifications in margins or other styles coming from Card.tsx, I moved the Overlay logic inside Card component. This feature will be enable is a overlayChildren prop is not null and only Exercise can pass that prop for now. I tested that overlay jumps when hover all the card:
Screen.Recording.2025-01-23.at.2.30.54.PM.mov
src/components/Exercise/index.tsx
Outdated
| <div ref={container}> | ||
| <div | ||
| ref={hoverRef} | ||
| tabIndex={enableOverlay ? 0 : -1} // This container is focusable only if enableOverlay is true |
There was a problem hiding this comment.
Does this cause any side effects for non-overlay exercises in screenreaders? I'm wondering if we should be more conservative and not set the attribute at all for normal use.
There was a problem hiding this comment.
Sorry for all the review rounds, but I just tested this with a screenreader and realized we need to make sure the content under the overlay is not focusable while the overlay is active.
It might also be nice to auto-focus the first control when it activates from tabbing forward only so it doesn't require as many tab presses to navigate.
|
@jomcarvajal Fixed my comment, did not mean to mention skipping announcing, as long as the overlay buttons are also read |
…nteractive element on overlay
Card: https://openstax.atlassian.net/browse/CORE-675
Changes for Exercise Preview card added. New properties to active Overlay and new component IncludeRemoveQuestion with 2 variants.
Screen.Recording.2025-01-17.at.4.14.04.PM.mov