Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alert has hard-coded title for screen reader #2023

Closed
ssilvert opened this issue May 16, 2019 · 9 comments
Closed

Alert has hard-coded title for screen reader #2023

ssilvert opened this issue May 16, 2019 · 9 comments
Assignees
Labels
Milestone

Comments

@ssilvert
Copy link

@ssilvert ssilvert commented May 16, 2019

Alerts should not have a hard-coded, unlocalizable title. It looks like this was addressed in #601, but now the problem is back?

Now we have:

  variantLabel = variantLabel || capitalize(AlertVariant[variant]);
  const readerTitle = (
    <React.Fragment>
      <span className={css(accessibleStyles.screenReader)}>{`${variantLabel} alert:`}</span>
      {title}
    </React.Fragment>
  );```
@redallen

This comment has been minimized.

Copy link
Contributor

@redallen redallen commented May 16, 2019

Please check if the logic in #1978 solves your problem. Here's the file:

export const Alert: React.FunctionComponent<AlertProps> = ({
  variant,
  variantLabel = capitalize(variant),
  'aria-label': ariaLabel = `${capitalize(variant)} Alert`,
  action = null,
  title,
  children = '',
  className = '',
  ...props
}: AlertProps) => {
  const readerTitle = (
    <React.Fragment>
      <span className={css(accessibleStyles.screenReader)}>{`${variantLabel} alert:`}</span>
      {title}
    </React.Fragment>
  );

  const customClassName = css(styles.alert, getModifier(styles, variant, styles.modifiers.info), className);

  return (
    <div {...props} className={customClassName} aria-label={ariaLabel}>
      <AlertIcon variant={variant} />
      <h4 className={css(styles.alertTitle)}>{readerTitle}</h4>
      {children && (
        <div className={css(styles.alertDescription)}>
          <p>{children}</p>
        </div>
      )}
      {action && (
        <div className={css(styles.alertAction, className)}>{React.cloneElement(action as any, { title, variantLabel })}</div>
      )}
    </div>
  );
};
@ssilvert

This comment has been minimized.

Copy link
Author

@ssilvert ssilvert commented May 16, 2019

From reading the code, it looks like there is still the hard-coded {${variantLabel} alert:}. That should not be there.

I don't want my alert to say "Success alert:"

@tlabaj

This comment has been minimized.

Copy link
Contributor

@tlabaj tlabaj commented May 16, 2019

@zach can you please fix this as part of your Pr you already have up.

@redallen

This comment has been minimized.

Copy link
Contributor

@redallen redallen commented May 16, 2019

@ssilvert I just updated my PR to use:

<span className={css(accessibleStyles.screenReader)}>
        {variantLabel ? variantLabel : `${capitalize(variant)} alert:`}
</span>
@redallen redallen changed the title Alert has hard-coded title Alert has hard-coded title for screen reader May 16, 2019
redallen added a commit to redallen/patternfly-react that referenced this issue May 16, 2019
@ssilvert

This comment has been minimized.

Copy link
Author

@ssilvert ssilvert commented May 16, 2019

IMO, this is still wrong. Under no circumstances should the Alert have an unlocalizable, hard-coded message. Just take the message passed in and display it.

It's not just for screen reader. It's for everything. No hard-coded messages should ever be displayed to the user.

@redallen

This comment has been minimized.

Copy link
Contributor

@redallen redallen commented May 16, 2019

With the new logic, we do take the message passed in (which is variantLabel) and display it. The old logic was wrong. We just set defaults for those who don't set a variantLabel or aria-label.

@ssilvert

This comment has been minimized.

Copy link
Author

@ssilvert ssilvert commented May 16, 2019

I don't want a variantLabel to be displayed. And I certainly don't want an unlocalized, hard-coded one to be displayed.

I just want to pass in a message and have it displayed exactly as I specified.

BTW, the documentation for these properties do not show up here https://patternfly-react.surge.sh/patternfly-4/components/alert/

@redallen

This comment has been minimized.

Copy link
Contributor

@redallen redallen commented May 16, 2019

The PR that is out will have the exact string you pass it displayed. It will also fix the documentation: https://1978-pr-patternfly-react-patternfly.surge.sh/patternfly-4/components/alert/

If you don't want default aria-label or variantLabels displayed that have hard-coded text, you can pass an empty string to override the default behavior. If you want to make the argument that defaults should not exist by default, open a separate issue that applies to all components.

@ssilvert

This comment has been minimized.

Copy link
Author

@ssilvert ssilvert commented May 16, 2019

I think that a developer expects to just pass in a message and have that message displayed as-is without modification. Without the docs, it was even more confusing as I didn't know the variantLabel param even existed.

As long as the docs tell you how to disable the default behavior, it should be ok. I would add that to the description of the variantLabel and aria-label props.

@tlabaj tlabaj closed this in 72a86f3 May 17, 2019
@rachael-phillips rachael-phillips added this to the 1.0.0-rc.2 milestone Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.