patternfly-react icon indicating copy to clipboard operation
patternfly-react copied to clipboard

feat(LoginPage): added headerUtilities prop for language selector example

Open andyyvo opened this issue 3 years ago • 1 comments

What: Closes #7228 .

How: Added a headerUtilities prop in LoginMainHeader.tsx using the styling properties discussed by @mcoker and @mcarrano. Updated LoginPage.tsx with a loginHeaderUtils prop. Created an example (LoginPageLanguageSelect.tsx) to demonstrate the new Select component. This example does not change the language of the login page at the moment.

Additional issues: N/A.

andyyvo avatar Aug 04 '22 16:08 andyyvo

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

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

patternfly-build avatar Aug 04 '22 16:08 patternfly-build

per @thatblindgeye also removed backgroundImgAlt on the other examples

andyyvo avatar Aug 16 '22 22:08 andyyvo

@tlabaj sounds good to me. The language selector could also be a demo. Seems like the show/hide password is maybe a demo, too?

mcoker avatar Aug 17 '22 00:08 mcoker

After a convo with Andy yesterday and looking into things a bit, the backgroundImgAlt prop on the LoginPage component isn't doing what it's intended to do, though it also seems unnecessary since the BackgroundImage component seems like it's meant strictly as decorative content. Since the alt attribute is being applied to a div in the BackgroundImage component, that alt text isn't being utilized in anyway that I can tell (not announced by screen reader nor rendering as text if the image fails to load).

I can open a separate issue (and mark it as a breaking change if necessary), but two possible options are:

  1. Remove the backgroundImgAlt prop from LoginPage (breaking, though not urgent so waiting for a breaking change release should be okay)
  2. If we intend for consumers to use the BackgroundImage component in a way that "alt text" should be exposed for assistive technologies, adding role="img" to BackgroundImage and using aria-label instead of the alt attribute for the alt text in LoginPage (possibly breaking, depending if consumers are utilizing this alt attribute?)

thatblindgeye avatar Aug 17 '22 16:08 thatblindgeye

After a convo with Andy yesterday and looking into things a bit, the backgroundImgAlt prop on the LoginPage component isn't doing what it's intended to do, though it also seems unnecessary since the BackgroundImage component seems like it's meant strictly as decorative content. Since the alt attribute is being applied to a div in the BackgroundImage component, that alt text isn't being utilized in anyway that I can tell (not announced by screen reader nor rendering as text if the image fails to load).

I can open a separate issue (and mark it as a breaking change if necessary), but two possible options are:

  1. Remove the backgroundImgAlt prop from LoginPage (breaking, though not urgent so waiting for a breaking change release should be okay)
  2. If we intend for consumers to use the BackgroundImage component in a way that "alt text" should be exposed for assistive technologies, adding role="img" to BackgroundImage and using aria-label instead of the alt attribute for the alt text in LoginPage (possibly breaking, depending if consumers are utilizing this alt attribute?)

@thatblindgeye I think you can open a breaking change follow up issue to remove the backgroundImgAlt

nicolethoen avatar Aug 18 '22 19:08 nicolethoen

@thatblindgeye I do not know which would be the better solution. Maybe open the breaking and pretty much copy the comment you have above and list both options. When we are implementing solution we can discus further. Or maybe get @jessiehuff's opinion in he issue.

tlabaj avatar Aug 18 '22 20:08 tlabaj