fix: add itinerary headingAs prop#910
fix: add itinerary headingAs prop#910nikkisato wants to merge 4 commits intoopentripplanner:masterfrom
Conversation
|
@miles-grant-ibigroup This is ready for review! |
miles-grant-ibigroup
left a comment
There was a problem hiding this comment.
Works without regressions, impressive!
Still, I think this should be released as a breaking change since it changes the default HTML output. To do this, create a new commit that makes a small negligible change. The title could be something like fix: add itinerary headingAs prop (as long as the fix: is present) and the body could be BREAKING CHANGE: changes printable-itinerary and itinerary-body output html tags (as long as BREAKING CHANGE: is present)
40eebb0 to
7ff2489
Compare
|
I'm sorry for the delay in looking at this! There's an unrelated bug right now in stories that's making it hard to review, but once that's resolved I'll look at it first thing |
amy-corson-ibigroup
left a comment
There was a problem hiding this comment.
Hello! Thank you for your patience. Would you mind providing a little more explanation for this change? I love the idea of using more semantic html to allowed screenreaders to parse more easily through the itineraries, but I have some questions about using headers to do so:
- I'm not sure it's desirable for the headers to be different for each type of leg. For example, an
h2on atransitLegfollowed by anh3on anaccessLegcould create an impression that the access leg is a nested part of the transit leg, which seems unnecessarily confusing. All legs in an itinerary should probably have the same hierarchical level of importance. - Wondering what your perspective would be on using
olas an alternative solution? Keeping the dynamic header for thetitleHeadingis good, but it makes more sense to me to have the legs presented in an ordered list a user could step through. I am fine with the headings approach though if you prefer it! I would just ask for the headings to be consistent throughout the itinerary. Right now I'm getting this readout, which feels a little confusing:
As always, thank you for the PR and I look forward to hearing more! It's exciting to have someone regularly contributing from an accessibility perspective!
|
Hi @amy-corson-ibigroup , Thanks so much for the thoughtful feedback — I really appreciate you digging into the semantic structure here and raising these concerns. I completely agree with you on the hierarchy point though — all legs should have the same level of importance, and the markup shouldn’t imply nesting where there isn’t any. The mixed heading levels you’re seeing right now are actually my mistake — I was testing this in the TriMet app and carried that over unintentionally. I’ll get that fixed in OTP-UI so the headings are consistent across all legs. One important bit of context is that we’ve tested the standard itinerary with disabled users, including screen reader users, and a consistent request from those sessions was to have a heading at the start of each itinerary leg. That feedback is what originally led us toward using headings rather than relying on list semantics alone. Best, |
a53a715 to
2877038
Compare
|
@amy-corson-ibigroup @miles-grant-ibigroup Fixing the heading levels I went and checked the storybooks and They look good to me please let me know what other adjustments i need to make thank you |
|
@amy-corson-ibigroup Hi Amy is there anything else you would need from me on this PR? |
|
Hi @nikkisato, apologies for the delay on this! I have been out of the office for the past couple of week, but I am back now and will definitely take a look at this this week! Thank you so much for the patience, I'll let you know if there are any issues ASAP |

BREAKING CHANGE: changes printable-itinerary and itinerary-body output html tags
Made the PrintableItinerary more flexible by using a polymorphic component idea of using
on <PrintableItinerary
legHeadings = {
titleHeading: "h2",
transitLeg: "h3",
tncLeg: "h3",
accessLeg: "h3"
}
/>
it would be really easy for someone to change the titleHeading from "h2" to "h1" and I did add a fallback on the Components for , , PageTitle, and TransitLeg
I wasn't sure how other people use this PrintableItinerary so tried to set it up as flexible as I can for accessibility
I verified it through storybook book by looking in the dev tools elements checking all the OTP-ui PrintableItinerary https://www.opentripplanner.org/otp-ui/?path=/story/printableitinerary--walk-only-itinerary&globals=locale:en-US
please let me know if there is any improvements to be made!
Have ran
pnpm test,pnpm update-snapshots,pnpm prepublish,pnpm build:ems,pnpm pack-all