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.

Sign up for GitHub

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

Jump to bottom

DSD-1867: overflow focus indicator #1701

Merged
merged 11 commits into from
Nov 26, 2024
Merged

DSD-1867: overflow focus indicator #1701

merged 11 commits into from
Nov 26, 2024

Conversation

oliviawongnyc
Copy link
Collaborator

@oliviawongnyc oliviawongnyc commented Nov 12, 2024 •
edited by jira bot
Loading

Fixes JIRA ticket DSD-1867

This PR does the following:

  • Fixes issue where focus indicator was being cut off in places in the Template component.

How has this been tested?

  • Locally in Storybook

Accessibility concerns or updates

  • Improves accessibility by improving the integrity of the focus indicator

Accessibility Checklist

  • Checked Storybook's "Accessibility" tab for color contrast and other issues.
  • The feature works with keyboard inputs including tabbing back and forward and pressing space, enter, arrow, and esc keys.
  • For hidden text or when aria-live is used, a screenreader was used to verify the text is read.
  • For features that involve UI updates and focusing on DOM refs, focus management was reviewed.
  • The feature works when the page is zoomed in to 200% and 400%.

Open Questions

Checklist:

  • I have updated the Storybook documentation and changelog accordingly.
  • I have added relevant accessibility documentation for this pull request.
  • All new and existing tests passed.

Front End Review:

  • Review the Vercel preview deployment once it is ready.

@vercel Vercel
Copy link

vercel bot commented Nov 12, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nypl-design-system ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 26, 2024 7:03pm

@oliviawongnyc oliviawongnyc added the Needs Review Pull requests that are ready for peer review. label Nov 12, 2024
@EdwinGuzman EdwinGuzman added the DON’T MERGE YET When somethign has been approved, but should not be merged just yet. label Nov 15, 2024
bigfishdesign13
bigfishdesign13 requested changes Nov 15, 2024
View reviewed changes
Copy link
Collaborator

@bigfishdesign13 bigfishdesign13 left a comment

Choose a reason for hiding this comment

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

The spacing for mobile is better, but desktop needs some adjustments.

CHANGELOG.md
@@ -8,6 +8,10 @@ Currently, this repo is in Prerelease. When it is released, this project will ad

## Prerelease

### Fixes

- Fixes issue where focus indicator was being cut off in places in the `Template` component.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel this should mention some details about the solution. Something about adjusting the padding throughout.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps in the specific component changelog?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated component changelog

src/theme/components/template.ts Outdated Show resolved Hide resolved
src/theme/components/template.ts Outdated Show resolved Hide resolved
EdwinGuzman
EdwinGuzman requested changes Nov 15, 2024
View reviewed changes
Copy link
Member

@EdwinGuzman EdwinGuzman left a comment

Choose a reason for hiding this comment

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

This makes the sidebar narrower. It's noticeable when comparing this against production. This does fix the focus issue but we should be wary of an update like this because it's a visual change for applications. If it should be narrower, we should give teams a heads up and aim to release this later on.

@oliviawongnyc
Copy link
Collaborator Author

This makes the sidebar narrower. It's noticeable when comparing this against production. This does fix the focus issue but we should be wary of an update like this because it's a visual change for applications. If it should be narrower, we should give teams a heads up and aim to release this later on.

@EdwinGuzman I believe @bigfishdesign13 and I resolved this by adding additional width to the sidebar column.

bigfishdesign13
bigfishdesign13 approved these changes Nov 18, 2024
View reviewed changes
Copy link
Collaborator

@bigfishdesign13 bigfishdesign13 left a comment

Choose a reason for hiding this comment

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

The spacing looks good now and the focus ring is fully visible. What do you think about an rc for additional testing in Turbine and perhaps elsewhere?

@EdwinGuzman EdwinGuzman removed the DON’T MERGE YET When somethign has been approved, but should not be merged just yet. label Nov 18, 2024
@EdwinGuzman EdwinGuzman added Ship It Pull requests that have been reviewed and approved. and removed Needs Review Pull requests that are ready for peer review. labels Nov 18, 2024
@EdwinGuzman EdwinGuzman added DON’T MERGE YET When somethign has been approved, but should not be merged just yet. and removed DON’T MERGE YET When somethign has been approved, but should not be merged just yet. labels Nov 20, 2024
EdwinGuzman
EdwinGuzman reviewed Nov 22, 2024
View reviewed changes
src/theme/components/template.ts Outdated
@@ -68,27 +69,35 @@ const TemplateContentTopBottom = defineStyleConfig({
const TemplateContentPrimary = defineStyleConfig({
baseStyle: defineStyle({
gridColumn: { base: "1", md: "1 / span 2" },
overflow: { base: "unset", md: "hidden" },
Copy link
Member

Choose a reason for hiding this comment

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

This is where the issue @bigfishdesign13 flagged comes from in https://github.com/NYPL/nypl-ds-test-app/pull/119#issuecomment-2494008501

bigfishdesign13
bigfishdesign13 approved these changes Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@bigfishdesign13 bigfishdesign13 left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@oliviawongnyc oliviawongnyc merged commit bd12ef8 into development Nov 26, 2024
5 checks passed
@oliviawongnyc oliviawongnyc deleted the DSD-1867/overflow-focus-indicator branch November 26, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EdwinGuzman EdwinGuzman EdwinGuzman approved these changes

@bigfishdesign13 bigfishdesign13 bigfishdesign13 approved these changes

Assignees
No one assigned
Labels
Ship It Pull requests that have been reviewed and approved.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oliviawongnyc @EdwinGuzman @bigfishdesign13