chore(Navigation): convert examples to TypeScript

[thatblindgeye]

What: Closes #7583

Navigation preview build
I removed the "Expandable (w/subnavigation titles)" example as the only difference between it and the "Expandable" example looked to just be the srText prop being passed in.

For the Flyout example, I passed in null as the children for the initial curFlyout definition, as TS throws an error about "Property 'children' is missing in type '{ depth: number; }' but required in type '{ depth: any; children: any; }'". We could also pass in another menu here to act as the last menu of the flyout chain, or do something similar to the Drilldown example and move some of the FlyoutMenu code outside of the exported NavFlyout component and create an interface for its props.

Additional issues:

[patternfly-build]

Preview: https://patternfly-react-pr-7597.surge.sh

A11y report: https://patternfly-react-pr-7597-a11y.surge.sh

[nicolethoen]

move some of the FlyoutMenu code outside of the exported NavFlyout component

This seems like a wise, modular idea. The more we can decouple the complex logic for flyouts (and drilldown) from the heart of these very important components, the easier that feels for maintainability.

[wise-king-sullyman]

It all looks good, but I'm a bit curious about your removal of the expandable w/ subnav titles example.

Do you think the demonstration of that prop is just wholly unnecessary?

[thatblindgeye]

It all looks good, but I'm a bit curious about your removal of the expandable w/ subnav titles example.

Do you think the demonstration of that prop is just wholly unnecessary?

When I originally removed it, it was mainly because the way the example was originally setup, the demonstration of the prop seemed unnecessary. Looking again, the example could be added back in I think if we more properly demonstrate the usage of the srText prop, but right now the result with VO doesn't exactly match with the description of the prop. "If defined, screen readers will read this text instead of the list title" is the description, but using VO the title still gets announced.

So I think ultimately it may depend on:

• Do we expect users to ever have an expandable Nav item without a title (so just an empty toggle except for the caret icon), but with visually hidden text via srText? If so, we would need to make the title attribute optional (I don't think this would be the best approach, though).
• Do we just want the srText prop to override the title prop for assistive technologies? If so, it may require some changes to the NavExpandable component (really just moving the srText to the expandable toggle, putting the title in a span/div, and making that title element aria-hidden)
• Do we ultimately need the srText prop since the title is required? If so, and none of the above apply, would it make more sense to have it act as supplementary information rather than the main label of the expandable nav item?

[wise-king-sullyman]

@thatblindgeye from testing it I think the real value of srText is that it allows something similar in nature to an aria label to describe the items in the second level nav for screen reader users, in a way that I think is really meant to compliment the title rather than replacing it. If I'm correct in that I think the best course of action would just be to update the prop description. Hard to say for sure though as the prop has been part of Nav from the beginning from the looks of it.

As far as needing an example for it or not I think the example does provide some value, but I wouldn't be against just adding it into the normal expandable nav demo. I do think it would be good to demonstrate it in some way though.

@nicolethoen @tlabaj do you all have any thoughts on the topic / any context on it Eric and I might be lacking?

[nicolethoen]

@wise-king-sullyman @thatblindgeye I don't have any extra context on Nav or it's history.

I don't feel attached to the example, so if we can demonstrate the value of the srText prop another way, that's fine with me. But since the title of an expandable navitem is required and it's a string, so there cannot be an icon or image, then i can't think of a use case for having more hidden text for screen reader users only...

[jessiehuff]

So I'm not sure about the history of the prop either, but I reached out to @jgiardino to see if she could provide any additional insight. Looking at the example, it seems like the hidden heading is there to provide some kind of additional context that might already be obvious to sighted users?

[thatblindgeye]

Looking at the example, it seems like the hidden heading is there to provide some kind of additional context that might already be obvious to sighted users?

That's what it seems like. I could possibly see a use-case for this prop of passing in additional info for an expandable group if it would result in the expandable title being too long, but at that point I would imagine such info shouldn't be visually hidden for only assistive tech. Otherwise I'm in the same boat as Nicole in not being able to see a use-case.

[jessiehuff]

Yeah, I agree. I checked in with Jenn as well, and she wasn't sure where the requirements for srText came from or if those original requirements are still valid. She said:

I think it came from feedback from Mark Caron that ul elements always have a heading element that is used as a label. I’m not sure if that’s a pattern that really makes sense in the context of our nav component, though. And there are things about it that seem odd, like having heading elements that are hidden. If it’s possible to eliminate that option, I would just use the visible text in the nav group toggle to label the ul elements that have the nav links.

So I think it's probably ok to remove that example for now unless we can come up with an example that demonstrates the prop in a useful way.

[tlabaj]

LGTM

[wise-king-sullyman]

🚀