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

cleanup: Remove detour-route-selection and route-ladder-header-update test groups #2697

Merged
merged 4 commits into from
Jul 24, 2024
Merged

cleanup: Remove detour-route-selection and route-ladder-header-update test groups #2697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a769efe
cleanup: Remove detour-route-selection and route-ladder-header-update…
joshlarson Jul 16, 2024
1766383
fixup! cleanup: Remove detour-route-selection and route-ladder-header…
joshlarson Jul 16, 2024
c3e70a6
fixup! cleanup: Remove detour-route-selection and route-ladder-header…
joshlarson Jul 17, 2024
5f54a0a
fixup! cleanup: Remove detour-route-selection and route-ladder-header…
joshlarson Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions assets/src/components/detours/diversionPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { DetourFinishedPanel } from "./detourFinishedPanel"
import { DetourRouteSelectionPanel } from "./detourRouteSelectionPanel"
import { Route, RoutePattern } from "../../schedule"
import RoutesContext from "../../contexts/routesContext"
import inTestGroup, { TestGroups } from "../../userInTestGroup"

const displayFieldsFromRouteAndPattern = (
route: Route,
Expand Down Expand Up @@ -203,11 +202,7 @@ export const DiversionPage = ({
routeDirection={routeDirection ?? "??"}
detourFinished={reviewDetour !== undefined}
onReviewDetour={reviewDetour}
onChangeRoute={
inTestGroup(TestGroups.DetourRouteSelection)
? () => send({ type: "detour.route-pattern.open" })
: undefined
}
onChangeRoute={() => send({ type: "detour.route-pattern.open" })}
/>
) : snapshot.matches({ "Detour Drawing": "Share Detour" }) &&
editDetour ? (
Expand Down
2 changes: 1 addition & 1 deletion assets/src/components/detours/diversionPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export interface DiversionPanelProps {
routeDirection: string
detourFinished?: boolean
onReviewDetour?: () => void
onChangeRoute?: () => void
onChangeRoute: () => void
}

export const DiversionPanel = ({
Expand Down
83 changes: 17 additions & 66 deletions assets/src/components/routeLadder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
import React, { useId } from "react"
import {
AlertIcon,
CrowdingIcon,
ReverseIcon,
ReverseIconReversed,
} from "../helpers/icon"
import { CrowdingIcon, ReverseIcon, ReverseIconReversed } from "../helpers/icon"
import {
getLadderCrowdingToggleForRoute,
LadderCrowdingToggle,
Expand All @@ -25,7 +20,6 @@ import { LoadableTimepoints, Route, RouteId } from "../schedule.d"
import IncomingBox from "./incomingBox"
import Ladder from "./ladder"
import Loading from "./loading"
import { CloseButton as OldCloseButton } from "./closeButton"
import Tippy from "@tippyjs/react"
import { tagManagerEvent } from "../helpers/googleTagManager"
import inTestGroup, { TestGroups } from "../userInTestGroup"
Expand All @@ -48,39 +42,6 @@ interface Props {
onAddDetour?: (route: Route) => void
}

export const Header = ({
routeName,
onClose,
hasAlert,
}: {
routeName: string
onClose: () => void
hasAlert: boolean
}) => {
return (
<div className="c-route-ladder__header">
{hasAlert && (
<Tippy
content="Active detour"
trigger="click"
onShow={() => tagManagerEvent("alert_tooltip_clicked")}
>
<AlertIcon
className="c-route-ladder__alert-icon"
aria-label="Route Alert"
/>
</Tippy>
)}
<div className="c-route-ladder__close-button-container">
<OldCloseButton closeButtonType="l_darker" onClick={onClose} />
</div>

<div className="c-route-ladder__route-name">{routeName}</div>
</div>
)
}

// TODO: delete old header after roll-out
export const NewHeader = ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about renaming NewHeader to Header in this PR? I think it should be renamed with this work, but understand if you'd want it in a new PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oooh additionally, beyond just changing the component name, the "new" label on the classnames will need changing. In hindsight, this should have been done in my work! I should have used the term "oldHeader" instead of going with "newHeader"

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 created a follow-up subtask for this in order to keep the diff for this smaller and easier to review. I could lump them together if you'd prefer.

routeName,
onClose,
Expand Down Expand Up @@ -130,8 +91,11 @@ export const NewHeader = ({
trigger="click"
onShow={() => tagManagerEvent("alert_tooltip_clicked")}
>
<div className="c-route-ladder__alert-icon">
<ExclamationTriangleFill aria-label="Route Alert" />
<div
className="c-route-ladder__alert-icon"
aria-label="Route Alert"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you teach me why the aria-label needed moving?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test uses a selector that tries to find the route alert icon using byRole("generic", { name: "Route alert" }). For reasons I don't fully understand, having the aria-label on the bootstrap icon doesn't actually put the aria-label onto the rendered component, but putting it on the wrapping div does.

I think this means that the new route ladder header route alert icon isn't doing accessibility quite right.

(FWIW - on main right now, the test is testing the old route ladder header, because the test group defaults to not being enabled, so the new route ladder header wasn't being tested for accessibility or an alert icon)

Copy link
Member

Choose a reason for hiding this comment

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

For reasons I don't fully understand, having the aria-label on the bootstrap icon doesn't actually put the aria-label onto the rendered component, but putting it on the wrapping div does.

You could try setting aria-hidden={false}

Copy link
Member

@firestack firestack Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But also, as I understand it, aria-label should be used as a last resort. This wasn't known when this was originally implemented, but a better way to do this would be to add the label text as a visually hidden DOM node and linking it to the icon with aria-labelledby

Copy link
Member

Choose a reason for hiding this comment

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

We could attempt to make things smarter, where if aria-label or aria-labelledby is provided, we could not set the default aria-hidden value, but I'm not sure it's worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but a better way to do this would be to add the label text as a visually hidden DOM node and linking it to the icon with aria-labelledby

Want to either:

  • pair on this?
  • ...address as a follow-up?

Copy link
Member

Choose a reason for hiding this comment

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

Want to either:

* pair on this?

* ...address as a follow-up?

I'm good either way! Since it was old code, I'm also fine with how it is for now.

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 tried aria-hidden={false} and it still said Unable to find an accessible element with the role "generic" and name "Route Alert"

I also tried aria-labelledby, but that gave this error:

Expected element to have accessible description:
  /Active Detour/i
Received:

PR (into this branch for now) below

>
<ExclamationTriangleFill />
</div>
</Tippy>
)}
Expand Down Expand Up @@ -251,30 +215,17 @@ const RouteLadder = ({

return (
<>
{inTestGroup(TestGroups.RouteLadderHeaderUpdate) ? (
<NewHeader
routeName={route.name}
hasAlert={hasAlert}
onClose={() => {
deselectRoute(route.id)
}}
showDropdown={
inTestGroup(TestGroups.DetoursPilot) &&
inTestGroup(TestGroups.DetourRouteSelection)
}
onClickAddDetour={() => {
onAddDetour?.(route)
}}
/>
) : (
<Header
routeName={route.name}
hasAlert={hasAlert}
onClose={() => {
deselectRoute(route.id)
}}
/>
)}
<NewHeader
routeName={route.name}
hasAlert={hasAlert}
onClose={() => {
deselectRoute(route.id)
}}
showDropdown={inTestGroup(TestGroups.DetoursPilot)}
onClickAddDetour={() => {
onAddDetour?.(route)
}}
/>
<Controls
displayCrowdingToggleIcon={displayCrowding}
ladderDirection={ladderDirection}
Expand Down
2 changes: 0 additions & 2 deletions assets/src/userInTestGroup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@ import getTestGroups from "./userTestGroups"

export enum TestGroups {
DemoMode = "demo-mode",
DetourRouteSelection = "detour-route-selection",
DetoursPilot = "detours-pilot",
DummyDetourPage = "dummy-detour-page",
KeycloakSso = "keycloak-sso",
MinimalLadderPage = "minimal-ladder-page",
LateView = "late-view",
RouteLadderHeaderUpdate = "route-ladder-header-update",
}

const inTestGroup = (key: TestGroups): boolean => {
Expand Down
25 changes: 0 additions & 25 deletions assets/stories/skate-components/routeLadderHeader.stories.tsx
View file
Open in desktop

This file was deleted.

Loading
Loading