fix(Page): improve a11y in sidebar and header

[boaz0]

What:

fixes #1136

//cc @jgiardino

[patternfly-build]

PatternFly-React preview: https://1145-pr-patternfly-react-patternfly.surge.sh

[coveralls]

Pull Request Test Coverage Report for Build 4103

• 2 of 3 (66.67%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.02%) to 80.36%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/patternfly-4/react-core/src/components/Page/PageHeader.js	1	2	50.0%

Totals
Change from base Build 4096:	-0.02%
Covered Lines:	4550
Relevant Lines:	5326

---

💛 - Coveralls

[jgiardino]

Thanks for working on this, @boaz0! Everything looks good, except one thing. When I click the hamburger button to toggle the display of the navigation panel, the aria-expanded attribute doesn't get updated. Currently it only says aria-expanded="true" regardless of the display state of the nav panel, but it should be set to "false" if the nav panel is hidden.

[boaz0]

@jgiardino I fixed it... thanks. 😃

[jgiardino]

Hmm, I'm still seeing this issue. I'm looking at the runtime html using the preview link above. Could the preview be stale?

[boaz0]

@jgiardino looking at surge.sh it seems to work:

[jgiardino]

Thanks for sharing that clip! I have been experiencing issues with the previews, so it's hard to tell if something is working as expected or not.

I am able to see the Page component work as expected, as shown in your example.

But when I look at the Page demo, I'm not seeing a change. But I'm also not sure if that's the correct expectation. I assumed that behavior is built into the Page component, and therefore would be visible wherever the Page component is being used, but I could be wrong about that .

cc'ing @jschuler on this in case he has input on that expectation, or to review this in case it is working and it's just me having issues with my preview again 😣

[boaz0]

Now I see what's the problem. I need to update the layout too. Thank you for noticing.

[boaz0]

@jgiardino fixed it -- can you take a look please?

[jgiardino]

Thanks! The PageLayoutDemo Looks good now!

And I had previously checked the Page component, which looked good, but now I'm noticing the following error for all components. Not sure if that has anything to do with changes in this PR or not.

[jgiardino]

Approving this since the html and attributes look good for the Page component, but there were recently errors showing on the component pages which may or may not need to be addressed here.

[boaz0]

That's really odd. Running it locally I don't see these errors. However, surge.sh still not being fixed even after doing commit --amend and push -f origin fixes_1136.

[jschuler]

That was an issue with the workspace in master that recently went in, should be fixed now after rebase

[dlabaj]

Wondering since isNavOpen is now set in the page header and page side bar if this makes more sense to set this in the Page component and pass it to the children.

[boaz0]

@dlabaj I thought about using React.cloneElement but since we are passing header and sidebar component rendered I am not sure it's possible without breaking the current API.

Let me know if I am forgetting something or you believe we can do it in a different way.

[tlabaj]

LGTM