Add Quotation Carousel #36

Merged
a258wang merged 12 commits from a258wang-quotation-carousel into main 5 months ago
Owner
Made without visx, because that was easier. Closes #10. https://a258wang-quotation-carousel-csc-class-profile-staging-snedadah.k8s.csclub.cloud/playground
a258wang added 10 commits 5 months ago
j285he approved these changes 5 months ago
j285he left a comment
Collaborator

Looks good to me. Figma's component has a slight gradient and the text is aligned left, but I think your version looks better.

Looks good to me. Figma's component has a slight gradient and the text is aligned left, but I think your version looks better.
}
.arrow {
position: relative;
Collaborator

Do you need this?

Do you need this?
Poster
Owner

Yes, it's required for the slight translation on hover to work.

Yes, it's required for the slight translation on hover to work.
a258wang marked this conversation as resolved
return (
<section
className={styles.carousel}
Collaborator

I noticed other components had customizable className props. Do we need it on this component as well? For example on Figma, the quotation has a subtle opacity change to show some circles in the background.

I noticed other components had customizable className props. Do we need it on this component as well? For example on Figma, the quotation has a subtle opacity change to show some circles in the background.
Poster
Owner

Good catch, I'll add that in. Also oops I got too excited to put up a PR and forgot to add the circles 🤦

Good catch, I'll add that in. Also oops I got too excited to put up a PR and forgot to add the circles 🤦
Poster
Owner

Added the customizable className prop, and also the background circles.

Added the customizable className prop, and also the background circles.
j285he marked this conversation as resolved
return (
<section
className={styles.carousel}
style={{ width: `${width / 16}rem`, minHeight: `${height / 16}rem` }}
Collaborator

I'm assuming that if we have very large text input, it's up to the developer to manually resize the carousel to fit everything, right?

I'm assuming that if we have very large text input, it's up to the developer to manually resize the carousel to fit everything, right?
Poster
Owner

Yes that's correct, making the developer resize the carousel using props ended up being the easiest for me to implement ("it's not my problem, it's yours" lol).

The issue I was having was getting all the cards to be the same size AND big enough for the longest quote in the carousel (even though the longest quote might be hidden).

Yes that's correct, making the developer resize the carousel using props ended up being the easiest for me to implement ("it's not my problem, it's yours" lol). The issue I was having was getting all the cards to be the same size AND big enough for the longest quote in the carousel (even though the longest quote might be hidden).
a258wang marked this conversation as resolved
<CarouselButton onClick={showPreviousCard} isPrevious />
<div className={styles.card}>
<QuotationMark className={styles.quotationMark} />
<ul>
Collaborator

If there is only ever one quote displayed at a time, why can't you replace the entire <ul> with

<p>{data[activeIdx]}</p>

Maybe I'm missing something here.

If there is only ever one quote displayed at a time, why can't you replace the entire `<ul>` with ``` <p>{data[activeIdx]}</p> ``` Maybe I'm missing something here.
Poster
Owner

I'm hiding quotes using visibility: hidden instead of display: none or conditional rendering, because it allows for a nicer fade animation when changing between quotes.

Also, accessibility best practices for carousels are kinda unclear, but one idea I saw was to make each thing in the carousel an item in a list. (This entire component could probably use an accessibility revisit though, to be honest. Actually, we should probably audit the entire Class Profile site for accessibility, once it's almost done.)

I'm hiding quotes using `visibility: hidden` instead of `display: none` or conditional rendering, because it allows for a nicer fade animation when changing between quotes. Also, accessibility best practices for carousels are kinda unclear, but one idea I saw was to make each thing in the carousel an item in a list. (This entire component could probably use an accessibility revisit though, to be honest. Actually, we should probably audit the entire Class Profile site for accessibility, once it's almost done.)
a258wang marked this conversation as resolved
a258wang added 2 commits 5 months ago
a258wang requested review from j285he 5 months ago
Collaborator

I suppose the Figma circles are not centered like this component is, so you could possibly think about adding the customizable props top and left to the circles, however I don't think it's necessary.

Looks great!!!

I suppose the Figma circles are not centered like this component is, so you could possibly think about adding the customizable props top and left to the circles, however I don't think it's necessary. Looks great!!!
j285he approved these changes 5 months ago
a258wang merged commit a2dbcb90c6 into main 5 months ago
a258wang deleted branch a258wang-quotation-carousel 5 months ago
a258wang referenced this issue from a commit 5 months ago
snedadah referenced this issue from a commit 5 months ago

Reviewers

j285he approved these changes 5 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as a2dbcb90c6.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#36
Loading…
There is no content yet.