www
/
cs-2022-class-profile
16
0
Fork 0
Code Issues 11 Pull Requests 1 Projects Releases Wiki Activity

Add Quotation Carousel #36

Merged
a258wang merged 12 commits from a258wang-quotation-carousel into main 2022-09-02 21:53:06 -04:00
Conversation 1 Commits 12 Files Changed 6 +299 -2
a258wang commented 2022-08-21 18:21:30 -04:00
Owner
Copy Link

Made without visx, because that was easier.

Closes #10.

https://a258wang-quotation-carousel-csc-class-profile-staging-snedadah.k8s.csclub.cloud/playground

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 2022-08-21 18:21:31 -04:00
continuous-integration/drone/push Build is passing Details
e3228f044f Fix carousel with long quotes by adding width/height props
j285he approved these changes 2022-08-25 18:45:23 -04:00
j285he left a comment
Member
Copy Link

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.
❤️ 1
components/QuotationCarousel.module.css Outdated
@ -0,0 +21,4 @@
}
.arrow {
position: relative;
j285he commented 2022-08-25 18:27:47 -04:00
Member
Copy Link

Do you need this?

Do you need this?
a258wang commented 2022-08-25 22:26:51 -04:00
Author
Owner
Copy Link

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

Yes, it's required for the slight translation on hover to work.
👍 1
a258wang marked this conversation as resolved
components/QuotationCarousel.tsx Outdated
@ -0,0 +31,4 @@
return (
<section
className={styles.carousel}
j285he commented 2022-08-25 18:24:30 -04:00
Member
Copy Link

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.
a258wang commented 2022-08-25 22:27:41 -04:00
Author
Owner
Copy Link

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 🤦
a258wang commented 2022-08-28 16:24:45 -04:00
Author
Owner
Copy Link

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
components/QuotationCarousel.tsx Outdated
@ -0,0 +32,4 @@
return (
<section
className={styles.carousel}
style={{ width: `${width / 16}rem`, minHeight: `${height / 16}rem` }}
j285he commented 2022-08-25 18:39:35 -04:00
Member
Copy Link

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?
a258wang commented 2022-08-25 22:31:46 -04:00
Author
Owner
Copy Link

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).
👍 1
a258wang marked this conversation as resolved
components/QuotationCarousel.tsx Outdated
@ -0,0 +37,4 @@
<CarouselButton onClick={showPreviousCard} isPrevious />
<div className={styles.card}>
<QuotationMark className={styles.quotationMark} />
<ul>
j285he commented 2022-08-25 18:34:58 -04:00
Member
Copy Link

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.
a258wang commented 2022-08-25 22:39:13 -04:00
Author
Owner
Copy Link

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.)
👍 1
a258wang marked this conversation as resolved
a258wang added 2 commits 2022-08-28 16:23:39 -04:00
continuous-integration/drone/push Build is passing Details
88e02f716b Tweak opacity and add aria-hidden to decorative elements
a258wang requested review from j285he 2022-08-28 16:24:59 -04:00
j285he commented 2022-08-31 22:43:14 -04:00
Member
Copy Link

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 2022-08-31 22:43:33 -04:00
a258wang merged commit a2dbcb90c6 into main 2022-09-02 21:53:06 -04:00
a258wang deleted branch a258wang-quotation-carousel 2022-09-02 21:53:06 -04:00
Sign in to join this conversation.
Reviewers
No reviewers
j285he
Labels
Clear labels   
Bug

   
Component

   
Config

   
Good First Issue

   
Low-Priority

   
Page
No Label
Bug
Component
Config
Good First Issue
Low-Priority
Page
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#36
No description provided.
Delete Branch "a258wang-quotation-carousel"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?