Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

💄(testimonies) new content impacting view: scroll through testimonies #30

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

manuhabitela
Copy link
Collaborator

since we're not sure on the actually wanted layout in figma I just made the wrapping div of testimonies overflow, styled a bit the scrollbar, and made it so that mouse users can "scroll" easily.

i guess this is acceptable for now and we change when we get specific layout info?

Peek.2024-03-21.18-32.mp4

@lebaudantoine
Copy link
Contributor

What you propose is acceptable for a first iteration.
What do you think of using the props slidesPerView on SwiperWrapper component. The SwiperWrapper will always be displayed, and we would update its props value based on the current window width. On desktop, the props slidesPerView would be equal to 3, the dot will indicate that more slides are available. On mobile the props slidesPerView would be equal to 1.

My suggestion might be a bit "hacky" and requiring too much JS for smth that could be done in pure HTML and CSS as you propose. Let's discuss it

@lebaudantoine
Copy link
Contributor

I discussed it with @johann, he would prefer the latter suggestion.

@manuhabitela
Copy link
Collaborator Author

Done!

And now with keyboard nav we can read all testimonies (was missing).

testimonies.mp4

@manuhabitela manuhabitela force-pushed the new-testimonies branch 3 times, most recently from a00d079 to 4462a05 Compare March 29, 2024 17:05
@manuhabitela
Copy link
Collaborator Author

manuhabitela commented Mar 29, 2024

Just need to remove my superfluous yarn.lock change done

@manuhabitela manuhabitela requested review from lebaudantoine and removed request for lebaudantoine March 29, 2024 17:11
@@ -20,7 +20,10 @@ interface CardProps {
}

const Card = ({ title, quote, img, entity }: CardProps) => (
<div className="flex flex-col bg-white p-7 text-left flex-1">
<div
tabIndex={0}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: when navigating using tab, the pagination disappears.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's that way for everytime we have this behavior. Could be improved for sure but the important thing is we can access everything.

add two new testimonies, making the layout needing more space so they
fit.

change UI logic on desktop: always use swiper, just show more slides at
the same time.
we tend to try to click them, lets just make them work. No need to make
them keyboard accessible from my point of view: having tabindexes on
items seems better UX (will have to test a bit more with screen readers
to be sure).
now keyboard users can view every testimony
@manuhabitela manuhabitela merged commit bffd614 into main Apr 2, 2024
3 checks passed
@manuhabitela manuhabitela deleted the new-testimonies branch April 2, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants