Skip to content

Conversation

@HedwigAR
Copy link
Contributor

@HedwigAR HedwigAR commented Nov 7, 2025

Pull request type

Bug fix (non-breaking change which fixes an issue)


Description

Added focus indicators to the navigation buttons in the carousel, disabled autoplay by default since it's not accessible.

What should be covered while testing?

  • Autoplay should be disabled in settings by default,
  • When tabbing to the navigation buttons on the carousel (the left and right arrows) a focus indicator should appear. The focus indicator should be blue and with a 2px offset, to match our other focus indicators.
  • The navigation buttons should also have a default white semi transparent circular background to make them stand out more.

@HedwigAR HedwigAR requested a review from a team as a code owner November 7, 2025 10:39

.swiper-button-next:focus,
.swiper-button-prev:focus {
outline: 2px solid #264ae5;
Copy link
Collaborator

@gjulivan gjulivan Dec 9, 2025

Choose a reason for hiding this comment

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

is there a css variables for this?
i think it should be var(--focus-outline)

.swiper-button-next:focus,
.swiper-button-prev:focus {
outline: 2px solid #264ae5;
outline-offset: 2px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

var(--focus-outline-offset)

--swiper-navigation-size: 24px;
border-radius: 50%;
background-color: rgba(245, 245, 245, 0.7);
padding: 2em;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use spacing small, large, or medium?

.swiper-button-prev {
--swiper-navigation-size: 24px;
border-radius: 50%;
background-color: rgba(245, 245, 245, 0.7);
Copy link
Collaborator

Choose a reason for hiding this comment

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

--gray-700 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but why that specifically?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This one is not specific to that. Maybe other color that is similar and currently available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used --gray-200 since that gave the right amount of color contrast

.swiper-button-next:focus,
.swiper-button-prev:focus {
outline: var(--focus-outline);
outline-offset: var(--focus-outline-offset);
Copy link
Collaborator

@gjulivan gjulivan Dec 10, 2025

Choose a reason for hiding this comment

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

you have to had fallback value... sass variables still needs to be defined.
otherwise, this will not work in atlas 3 with sass variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants