feat(LoginPage): added headerUtilities prop for language selector example by andyyvo · Pull Request #7793 · patternfly/patternfly-react

andyyvo · 2022-08-04T16:38:40Z

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.

…mple

patternfly-build · 2022-08-04T16:53:52Z

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

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

mcarrano

This looks good @andyyvo . But just one request. For a language selector, it's standard practice to show language names in their native language and character set, i.e., Espanol rather than Spanish, and Asian languages in their native font. It there an easy way to do this? I believe we had a code example in PatternFly 3 that implemented this menu.

mcoker

Looks awesome!! Just a few nits/questions - the only thing I think really needs updating is the initially selected language ("English" in the demo) being the selected item in the menu, too. Linked to another demo that shows how that might be wired up.

mcoker · 2022-08-04T20:28:12Z

  title?: string;
  /** Subtitle that contains the Text, URL, and URL Text for the Login Main Header */
  subtitle?: string;
+  /** Select menu that renders next to the Login Title for the Login Main Header */


nit - FWIW this could contain anything (likely actions of some sort), and it isn't always by the title, it's below the subtitle on small screens. I wonder if we could just describe this as a place for actions, such as a language selector, or something like that?

mcoker · 2022-08-04T20:36:53Z

@@ -12,13 +12,16 @@ export interface LoginMainHeaderProps extends React.HTMLProps<HTMLDivElement> {
  title?: string;
  /** Subtitle that contains the Text, URL, and URL Text for the Login Main Header */


out of scope but what does URL and URL Text refer to? This is a generic "description" element in core, and rendered as a <p>. Seems like it might be worth updating this prop to description (like the modal component) since subtitle implies it might be a heading element or something - it would be invalid invalid to pass a heading there currently since you can't put a heading in a <p>. On that, we might also consider just making this a <div> in both core and react. If that seems valid, I can open an issue.

I see what you mean. I could definitely see the user placing a hyperlink URL here. If you do open up that issue, feel free to tag me so I can make a react issue to update the prop name and usage

mcoker · 2022-08-04T20:37:53Z

  loginTitle: string;
  /** Subtitle for the Login Main Body Header of the LoginPage */
  loginSubtitle?: string;
+  /** Header utilities for the Login Main Body Header of the LoginPage */


nit - we might consider updating these descriptions to use sentence case?

should I update this for all of the descriptions?

yes - even our component names are lower cased when used in a sentence

mcoker · 2022-08-04T20:48:56Z

+  const [selectedHeaderUtils, setSelectedHeaderUtils] = React.useState<string | SelectOption>();
+
+  const headerUtilsOptions = [
+    <SelectOption key={0} value="English" />,


Can this be selected in the menu, too? FWIW I found this filterable table demo that has "status" as initially selected, and if you open the menu, it's selected in the menu, too.

thatblindgeye

This is looking good so far! In addition to what has been commented already, I had some suggested changes/comments below.

andyyvo · 2022-08-16T22:41:10Z

per @thatblindgeye also removed backgroundImgAlt on the other examples

tlabaj

@mcoker @mcarrano since the header utilities is technically a container for any action, I wonder if we want to specifically name the example "With language selector". A more generic name may be appropriate. Maybe we could add a description above the example to mention the header utilities should be used to add a language selector .

mcoker · 2022-08-17T00:50:49Z

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

thatblindgeye · 2022-08-17T16:16:26Z

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:

Remove the backgroundImgAlt prop from LoginPage (breaking, though not urgent so waiting for a breaking change release should be okay)
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?)

mcoker

lgtm!

tlabaj · 2022-08-18T13:33:03Z

 ```ts file='./LoginPageShowHidePassword.tsx' isFullscreen
 ```
+
+### With language selector


I would still suggest renaming this example and adding a description about how actions can be used for a language selector.
cc @mcarrano

I'm OK with that if it would clarify what this example is illustrating.

nicolethoen · 2022-08-18T19:00:02Z

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:

Remove the backgroundImgAlt prop from LoginPage (breaking, though not urgent so waiting for a breaking change release should be okay)

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

thatblindgeye

Other than comments above, looks good! Will open a follow up issue based on the above as well.

tlabaj · 2022-08-18T20:50:59Z

@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.

mcarrano

This looks great. Thanks @andyyvo !

tlabaj · 2022-08-19T18:17:34Z

  loginSubtitle?: string;
-  /** Content rendered inside of Login Main Footer Band to display a sign up for account message */
+  /** Header utilities for the login main body header of the login page */
+  headerUtilities?: React.ReactNode;


could we label this one beta? Then we can merge.

@tlabaj how do I label something as beta?

Hi @andyyvo You just need to but the @beta at the beginning of the prop description. This give us the flexibility to improve on API (including breaking change) if need be. Once we decide feature is stable, we promote it out of beta and remove the tag.
/** @beta Header utilities for the login main body header of the login page */

@tlabaj would you also like to see this on the LoginMainHeader.tsx prop as well?

tlabaj

LGTM!

feat(LoginPage): added headerUtilities prop for language selector exa…

c034c7c

…mple

andyyvo requested review from mcarrano, mcoker and nicolethoen August 4, 2022 16:38

mcarrano suggested changes Aug 4, 2022

View reviewed changes

mcoker requested changes Aug 4, 2022

View reviewed changes

thatblindgeye requested changes Aug 8, 2022

View reviewed changes

andyyvo added 2 commits August 9, 2022 23:55

feat(LoginPage): edits to props, descriptions, and hook types

27911c5

feat(LoginPage): removed backgroundImgAlt off examples

a6e582d

tlabaj reviewed Aug 16, 2022

View reviewed changes

tlabaj added ux review labels Aug 16, 2022

mcoker approved these changes Aug 17, 2022

View reviewed changes

tlabaj reviewed Aug 18, 2022

View reviewed changes

thatblindgeye approved these changes Aug 18, 2022

View reviewed changes

feat(LoginPage): changing markdown description

2c7c620

thatblindgeye mentioned this pull request Aug 18, 2022

Bug - LoginPage: backgroundImgAlt prop does not do what is intended #7867

Closed

mcarrano approved these changes Aug 19, 2022

View reviewed changes

tlabaj reviewed Aug 19, 2022

View reviewed changes

feat(LoginPage): making prop beta

f2cc5f9

tlabaj approved these changes Aug 23, 2022

View reviewed changes

tlabaj merged commit b902b2c into patternfly:main Aug 23, 2022

		@@ -12,13 +12,16 @@ export interface LoginMainHeaderProps extends React.HTMLProps<HTMLDivElement> {
		title?: string;
		/** Subtitle that contains the Text, URL, and URL Text for the Login Main Header */

Conversation

andyyvo commented Aug 4, 2022

Uh oh!

patternfly-build commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andyyvo commented Aug 16, 2022

Uh oh!

tlabaj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcoker commented Aug 17, 2022

Uh oh!

thatblindgeye commented Aug 17, 2022

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolethoen commented Aug 18, 2022

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj commented Aug 18, 2022

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlabaj Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

patternfly-build commented Aug 4, 2022 •

edited

Loading

tlabaj left a comment •

edited

Loading

tlabaj Aug 23, 2022 •

edited

Loading