ToggleButtonGroup and Carousel components created for Sections filters feature #66

jomcarvajal · 2025-01-22T17:14:30Z

Ticket: https://openstax.atlassian.net/browse/CORE-675
Design: https://www.figma.com/design/9xbDQWGZuE4vdYb8kqsyvj/instructor-side?node-id=3994-4223&t=BBN1G7YWDrkB6v2J-4

Storybook screenshots

Carousel:

Screen.Recording.2025-01-21.at.9.03.57.AM.mov

ToggleButtonGroup:

Carousel + ToggleButtonGroup:

Resize example:
https://github.com/user-attachments/assets/9110a109-94df-410f-bf51-ef1564662220

…s feature

jomcarvajal · 2025-01-22T17:15:13Z

src/components/__snapshots__/ErrorModal.spec.tsx.snap

@@ -36,6 +36,7 @@ exports[`Modal matches snapshot 1`] = `
        <section
          aria-labelledby="react-aria-2"
          class="sc-eCYdqJ iasutB"
+          data-rac=""


Updates coming from react-aria-components v1.6.0

jivey

This is looking really good, I'm mainly wondering about responsiveness and some minor things.

jivey · 2025-01-22T20:22:49Z

src/components/Carousel/styles.ts

+
+export const CarouselContainer = styled.div<{customWidth?: string}>`
+  position: relative;
+  width: ${({customWidth}) =>  customWidth ? customWidth : '30rem'};


We need this to be responsive, does it still work if the default was 100%?

I will leave it as 100%. Still working, I attach a video . Let me know if something is wrong

Screen.Recording.2025-01-22.at.3.09.21.PM.mov

I wasn't paying attention to the wrapper when testing this... I think we can default to not defining the width and let the block rendering do what we want. Otherwise the wrapping container will need to add some styles to contain it - at least on Firefox, 100% causes overflow: https://openstax.github.io/ui-components/refs/heads/CORE-675-filter-sections-component/c27809ccad34b4b7353423157e55a57d9578bef2/?mode=preview&story=carousel--default

So should I remove the customWidth as well ? Or keep it and have a width: auto as default ?

Yeah I think so, we can always let the consuming app define it by overriding the styled component

src/components/Carousel/index.tsx

jivey · 2025-01-22T20:30:43Z

src/components/Carousel/styles.ts

+  opacity: ${({ disabled }) => (disabled ? 0.5 : 1)};
+`;
+
+export const StyledRightArrow = styled(RightArrow)<{ disabled: boolean }>`


Can we add padding or width/height to these arrows so the tap target is bigger?

When you convert these to buttons that contain the svg, if you put 100% height on the button, but constrain the height on the svg, we should be able to match the container height without the svg size getting too big.

src/components/ToggleButtonGroup.stories.tsx

src/components/ToggleButtonGroup/index.tsx

jivey

The right arrow is initializing correctly now, but I think we need to listen for resize, because it doesn't update from disabled to enabled if you collapse the window enough to hide items. Random thought, but I wonder if using normal scroll overflow and a mutation observer that checks for it would simplify the arrow implementation 🤔

jivey · 2025-01-23T21:22:27Z

src/components/Carousel/index.tsx


        }
-      }, [currentIndex, wrapperRef]);
-
+    }, [currentIndex, wrapperRef]);


Looks like this repo don't fail the build if the linter doesn't pass, oops. Anyway it wants checkIfRightArrowShouldBeDisabled in the dependency array

My bad, didn't run the linter this time

jivey · 2025-01-23T21:23:37Z

src/components/Carousel/index.tsx

-          setIsRightArrowDisabled(isDisabled);
-          setIsLeftArrowDisabled((currentIndex) === 0);
+            checkIfRightArrowShouldBeDisabled()
+            window.addEventListener('resize', checkIfRightArrowShouldBeDisabled);


The effect needs to run removeEventListenter on return

jivey · 2025-01-24T17:34:22Z

src/components/Carousel/index.tsx

+                    })}
+                </CarouselWrapper>
+            </CarouselOverflow>
+            <StyledRightArrow


I didn't notice it before, but these need to be real button elements that can be focused

jivey · 2025-01-24T17:37:16Z

src/components/Carousel/styles.ts

+  opacity: ${({ disabled }) => (disabled ? 0.5 : 1)};
+`;
+
+export const StyledRightArrow = styled(RightArrow)<{ disabled: boolean }>`


When you convert these to buttons that contain the svg, if you put 100% height on the button, but constrain the height on the svg, we should be able to match the container height without the svg size getting too big.

jivey

This is basically good to go I just have some tiny tweaks

jivey · 2025-01-30T15:46:28Z

src/components/ToggleButtonGroup/styles.ts

+    height: 3.6rem;
+    padding: 0.8rem;
+    background-color: ${colors.palette.white};
+    border: solid 0.1rem ${colors.palette.pale};


Tiny thing I didn't notice before: can we do a & + & rule here to remove the left border?

jivey · 2025-01-30T15:53:00Z

src/components/Carousel/styles.ts

+  background: transparent;
+
+  &:hover {
+    box-shadow: 0rem 0.2rem 0.4rem rgba(0, 0, 0, 0.2);


We don't have designs for this in Figma, but to fit into the overall flat design, can you remove this and make it a solid border?

So, add a solid border in buttons ?

Yeah just for the hover

Meaning, not for all buttons but just this rule right here

jivey · 2025-01-30T15:53:32Z

src/components/Carousel/styles.ts

+
+  svg {
+    position: relative;
+    cursor: ${({ disabled }) => (disabled ? 'not-allowed' : 'pointer')};


Can you move this to the button?

For this one, if we move it up one level, it'll look a little nicer if it activates on the button mouse over not the svg

…tax/ui-components into CORE-675-filter-sections-component

jivey

Left a comment about the border shifting and the color, feel free to merge if you change those and it looks good to you.

jivey · 2025-01-30T17:22:03Z

src/components/Carousel/styles.ts

+  opacity: ${({ disabled }) => (disabled ? 0.5 : 1)};
+
+  &:hover {
+    border: 0.1rem solid ${palette.black};


We use pale more often for borders

jivey · 2025-01-30T17:22:28Z

src/components/Carousel/styles.ts

+  position: absolute;
+  top: 50%;
+  transform: translateY(-50%);
+  border: none;


I think you want a transparent border here so the svg doesn't shift on hover

jivey · 2025-01-30T17:24:45Z

@jomcarvajal Oh yeah before you merge, can you also bump the package version? Then we can make a build off main once it's in.

ToggleButtonGroup and Carousel components created for Sections filter…

250f849

…s feature

jomcarvajal commented Jan 22, 2025

View reviewed changes

jomcarvajal requested a review from jivey January 22, 2025 18:26

jivey requested changes Jan 22, 2025

View reviewed changes

resolve comments, fix bug with disabled arrows

c27809c

github-actions bot deployed to refs/heads/CORE-675-filter-sections-component January 22, 2025 22:34 View deployment

jomcarvajal requested a review from jivey January 23, 2025 15:26

jivey requested changes Jan 23, 2025

View reviewed changes

resize update for arrows state added

16b20a1

github-actions bot deployed to refs/heads/CORE-675-filter-sections-component January 23, 2025 16:52 View deployment

jomcarvajal requested a review from jivey January 23, 2025 16:59

jivey requested changes Jan 23, 2025

View reviewed changes

return removeEventListener resize

42662b0

github-actions bot deployed to refs/heads/CORE-675-filter-sections-component January 23, 2025 21:36 View deployment

jivey requested changes Jan 24, 2025

View reviewed changes

arrows are now buttons containing svg left and right arrows

b6ed392

github-actions bot deployed to refs/heads/CORE-675-filter-sections-component January 27, 2025 15:33 View deployment

adjust arrows dimensions

3c8955d

github-actions bot deployed to refs/heads/CORE-675-filter-sections-component January 27, 2025 15:40 View deployment

missing exports

152ca39

github-actions bot deployed to refs/heads/CORE-675-filter-sections-component January 28, 2025 22:36 View deployment

Merge branch 'main' into CORE-675-filter-sections-component

3659d18

jomcarvajal requested a review from jivey January 30, 2025 13:43

github-actions bot deployed to refs/heads/CORE-675-filter-sections-component January 30, 2025 13:43 View deployment

jivey requested changes Jan 30, 2025

View reviewed changes

jomcarvajal added 2 commits January 30, 2025 11:08

fix styles in carousel and togglebuttongroup

4c1b18f

Merge branch 'CORE-675-filter-sections-component' of github.com:opens…

fab7d71

…tax/ui-components into CORE-675-filter-sections-component

github-actions bot deployed to refs/heads/CORE-675-filter-sections-component January 30, 2025 17:10 View deployment

jomcarvajal requested a review from jivey January 30, 2025 17:14

jivey approved these changes Jan 30, 2025

View reviewed changes

bump version 1.13.0

a4e20ce

github-actions bot deployed to refs/heads/CORE-675-filter-sections-component January 30, 2025 17:29 View deployment

update snapshots

fa6c3e1

github-actions bot deployed to refs/heads/CORE-675-filter-sections-component January 30, 2025 17:32 View deployment

jomcarvajal merged commit fc9ecbb into main Jan 30, 2025
3 checks passed

jomcarvajal deleted the CORE-675-filter-sections-component branch January 30, 2025 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ToggleButtonGroup and Carousel components created for Sections filters feature #66

ToggleButtonGroup and Carousel components created for Sections filters feature #66

jomcarvajal commented Jan 22, 2025 •

edited

Loading

jomcarvajal Jan 22, 2025

jivey left a comment

jivey Jan 22, 2025

jomcarvajal Jan 22, 2025

jivey Jan 23, 2025

jomcarvajal Jan 23, 2025 •

edited

Loading

jivey Jan 23, 2025

jivey Jan 22, 2025

jivey Jan 24, 2025

jivey left a comment

jivey Jan 23, 2025

jomcarvajal Jan 23, 2025

jivey Jan 23, 2025

jivey Jan 24, 2025

jivey Jan 24, 2025

jivey left a comment

jivey Jan 30, 2025

jivey Jan 30, 2025

jomcarvajal Jan 30, 2025

jivey Jan 30, 2025

jivey Jan 30, 2025

jivey Jan 30, 2025

jivey Jan 30, 2025

jivey left a comment

jivey Jan 30, 2025

jivey Jan 30, 2025

jivey commented Jan 30, 2025

ToggleButtonGroup and Carousel components created for Sections filters feature #66

ToggleButtonGroup and Carousel components created for Sections filters feature #66

Conversation

jomcarvajal commented Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

jivey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jomcarvajal Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jivey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jivey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jivey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jivey commented Jan 30, 2025

jomcarvajal commented Jan 22, 2025 •

edited

Loading

jomcarvajal Jan 23, 2025 •

edited

Loading