Conversation
There was a problem hiding this comment.
Pull request overview
This PR is part of the ongoing ADA compliance work, primarily improving keyboard focus visibility and focus affordances across route/stop cards, header/footer links, and the Google Translate trigger.
Changes:
- Introduces a reusable
UnderlineOnFocusElementhelper and applies it to multiple interactive labels/links. - Adds “group” focus styling patterns to card headers and header logo elements for consistent focus underline/border behavior.
- Updates Google Translate trigger icon styling to use
currentColorand adjusts focus styles across a few buttons/links.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/css/bustime.sass | Adds (commented) focus-decoration experiments inside card-header focus styling. |
| src/css/_helpers.sass | Normalizes a Sass variable line and adds a new translate-related color token. |
| src/components/views/StopCardWrapper.tsx | Applies grouped focus underline behavior to stop-direction header labels. |
| src/components/views/RouteCard.tsx | Applies grouped focus underline behavior to route-direction destination text. |
| src/components/views/FavoritesCard.tsx | Refactors favorite item rendering to use new selectable header/card components. |
| src/components/views/CardHeaderComponents.tsx | Adds selectable favorite route/stop card components using underline-on-focus behavior. |
| src/components/views/AllRoutesWrapper.tsx | Wraps abbreviated route card header with underline-on-focus helper. |
| src/components/shared/common.tsx | Adds UnderlineOnFocusElement and enhances ChangeViewButton focus ring/outline styling. |
| src/components/shared/buttons.tsx | Tweaks shuttle button focus outline styling. |
| src/components/pageStructure/Header.js | Adds focus border styling via Tailwind utilities to logo elements/links. |
| src/components/pageStructure/GoogleTranslate.tsx | Converts translate trigger to use underline-on-focus helper + currentColor SVG strokes. |
| src/components/pageStructure/Footer.js | Converts footer links to use underline-on-focus helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div className={`card route-card ${routeMatch.routeId}`} onClick={()=>search(routeMatch.routeId)}> | ||
| <button | ||
| className="group card-header link-header" | ||
| style={{ borderColor: "#" + routeMatch.color }} | ||
| tabIndex={0} |
There was a problem hiding this comment.
These new selectable favorite cards attach navigation onClick to a wrapping <div> while also rendering a <button> inside. This creates nested interactive behavior (and relies on event bubbling), which can confuse assistive tech and makes keyboard/mouse behavior harder to reason about. Prefer putting the click handler on the <button> (or render a single interactive element) and keep the wrapper purely presentational.
| <a href="#" onClick={(e) => {e.preventDefault(); search("")}} aria-label="MTA Bus Time Home" className="logo-link group"> | ||
| <img src={bustimeLogo} alt="MTA Bus Time" className="logo group-focus:border-mta-yellow border-b-3 border-transparent" /> |
There was a problem hiding this comment.
border-b-3 is not a standard Tailwind border-width utility (default is typically 0/2/4/8). If Tailwind isn’t configured to generate it, this focus indicator won’t render. Use a supported class (e.g., border-b-2/border-b-4) or an arbitrary value (border-b-[3px]) if you need 3px.
| <div className="w-px h-3/4 bg-mta-black-4 self-end"></div> | ||
|
|
||
| <a href="https://www.mta.info/" target="_blank" aria-label="MTA Home" className="bottom flex mta-logo-link justify-end items-end"> | ||
| <a href="https://www.mta.info/" target="_blank" aria-label="MTA Home" className=" bottom flex mta-logo-link justify-end items-end focus:border-mta-yellow border-b-3 border-transparent"> |
There was a problem hiding this comment.
This external link uses target="_blank" without rel="noopener noreferrer", which can enable reverse-tabnabbing. Add the appropriate rel attributes when opening a new tab/window.
| <a href="https://www.mta.info/" target="_blank" aria-label="MTA Home" className=" bottom flex mta-logo-link justify-end items-end focus:border-mta-yellow border-b-3 border-transparent"> | |
| <a href="https://www.mta.info/" target="_blank" rel="noopener noreferrer" aria-label="MTA Home" className=" bottom flex mta-logo-link justify-end items-end focus:border-mta-yellow border-b-3 border-transparent"> |
| function UnderlineOnFocusElement({ | ||
| as: Element = 'a', | ||
| variant = 'mta_yellow', | ||
| className, | ||
| children, | ||
| ...props | ||
| }: Props) { |
There was a problem hiding this comment.
UnderlineOnFocusElement is typed with Props, but Props is not declared or imported in this file. This will cause a TypeScript error (Cannot find name 'Props') and makes the component’s API unclear. Define a proper props type (e.g., a polymorphic prop type that includes as, variant, className, children, and the underlying element props) and use it here.
There was a problem hiding this comment.
ts cleanup is coming in another branch, accepting it for now
|
|
||
|
|
||
| export {ChangeViewButton, ChangeViewButtonProps} No newline at end of file | ||
| // // has multiple variants rather than accepting color as prop becasue tailwind struggles with dynamic classes |
There was a problem hiding this comment.
Typo in comment: “becasue” should be “because”.
| // // has multiple variants rather than accepting color as prop becasue tailwind struggles with dynamic classes | |
| // // has multiple variants rather than accepting color as prop because tailwind struggles with dynamic classes |
| // text-decoration: underline | ||
| // text-decoration-color: $black | ||
| // text-decoration-thickness: 2px |
There was a problem hiding this comment.
There are commented-out text-decoration focus styles left in the .card-header &:focus block. Since they’re not active, they add noise and make it unclear what the intended focus treatment is. Either remove them or re-enable them (if still needed) as part of the ADA focus styling work.
| // text-decoration: underline | |
| // text-decoration-color: $black | |
| // text-decoration-thickness: 2px | |
| text-decoration: underline | |
| text-decoration-color: $black | |
| text-decoration-thickness: 2px |
| <div className="w-px h-3/4 bg-mta-black-4 self-end"></div> | ||
|
|
||
| <a href="https://www.mta.info/" target="_blank" aria-label="MTA Home" className="bottom flex mta-logo-link justify-end items-end"> | ||
| <a href="https://www.mta.info/" target="_blank" aria-label="MTA Home" className=" bottom flex mta-logo-link justify-end items-end focus:border-mta-yellow border-b-3 border-transparent"> |
There was a problem hiding this comment.
border-b-3 is likely not generated by Tailwind’s default border-width scale. If it’s not configured, the intended focus border won’t appear here. Consider switching to a supported utility (e.g., border-b-2/border-b-4) or border-b-[3px].
| <div className={`card route-card ${routeMatch.routeId}`} onClick={()=>search(routeMatch.routeId)}> | ||
| <button | ||
| <UnderlineOnFocusElement | ||
| variant="black" | ||
| as="button" | ||
| className="card-header link-header" | ||
| style={{ borderColor: "#" + routeMatch.color }} | ||
| tabIndex={0} | ||
| > | ||
| <h3 className="card-title">{OBA.Config.noWidows(routeMatch.routeTitle)}</h3> | ||
| </button> | ||
| </UnderlineOnFocusElement> |
There was a problem hiding this comment.
AbreviatedRouteCard still puts the navigation onClick on the wrapping <div> while rendering a <button> without an onClick. This relies on event bubbling and results in a non-interactive container behaving like a control. For clearer semantics/accessibility, move the click handler onto the <button> (or render a single interactive element) and keep the wrapper non-clickable.
| import log from 'loglevel'; | ||
| import stopPopupIcon from "../../img/icon/bus-stop.svg" | ||
| import {StarBorderIcon, BusStopIcon, VehicleIcon} from "../shared/icons"; | ||
| import { SelectableFavoriteRouteCard, SelectableFavoriteStopCard } from "./CardHeaderComponents"; | ||
|
|
There was a problem hiding this comment.
After refactoring FavoriteItem to delegate rendering to SelectableFavoriteRouteCard / SelectableFavoriteStopCard, several imports and locals in this file appear unused (e.g., Cookies, OBA, stopPopupIcon, StarBorderIcon/BusStopIcon/VehicleIcon, and the removeFavorite/search destructures in FavoriteItem). Cleaning these up will reduce bundle noise and avoid unused-var tooling failures.
8f4447b to
8b06674
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
represents a set of changes needed for ada compliance
not finished but a review would help