Remove Paragraph Tag from Alert Body

[andrewnottaviano]

Bug Report

Description

Currently the terra-alert module wraps the body of the alert in a <p> tag. This proves problematic if a consumer uses HTML elements (which the docs state is valid input) in body of the alert as the react DOM validator outputs a warning. I have a attached a screenshot of the warning from the console below.

Steps to Reproduce

1. Create a Terra Alert in a test site.
2. Place a <div> element (or a Terra element that uses a <div> inside the alert as a child element.
3. Observe the warning in the console.

Additional Context / Screenshots

Expected Behavior

I would expect that when I use HTML elements (besides a <span>, for some reason it does not complain about a <span> tag) or other Terra elements that use divs under the hood to not print an error.

Possible Solution

Change the Terra Alert component to use a <div> for the body instead of a <p> tag.

Environment

• Component Name and Version: terra-alert 2.33.0
• Browser Name and Version: Google Chrome 70.0.3538.110
• Node/npm Version: Node 8.4 npm 5.3
• Webpack Version: 4
• Operating System and version (desktop or mobile): macOS desktop

@ Mentions

@tbiethman

[JakeLaCombe]

What use cases for using a paragraph instead of a div do you have? Switching from a paragraph to a div costs us accessibility with the alert.

[andrewnottaviano]

@JakeLaCombe Thanks for responding back. I am trying to build an alert that allows us to show and hide the message in the body of the alert when the user clicks a toggle button. There is a similar example on the Terra UI doc page (the long text example).

[JakeLaCombe]

I feel like that's not in alignment with the OCS design. For our alerts, we actually have a dedicated space for putting actions, and that would be on the right side of the alert container. Check the action portion of this page. https://engineering.cerner.com/terra-core/#/components/terra-alert/alert/alert

[JakeLaCombe]

Also note that in your use case, the button is still at the end. It's just dropped to a newline to compensate for the space needed for the text.

[andrewnottaviano]

I'm not sure that I have explained the situation clearly enough, my apologies. We are attempting to build an error banner that is similar in nature to the long text example cited in the docs. Here is the snippet from the source code that we are attempting to replicate:

<Alert type="info" title="Gettysburg Address:">
  <span>
    Four score and seven years ago our fathers brought forth on this continent, a new nation, conceived in Liberty, and dedicated to the proposition that all men are created equal.
    <Toggle isOpen={!this.state.allTextShown}>
      <Button onClick={this.handleShowMore} text="Show More" style={buttonStyle} />
    </Toggle>
    <Toggle isOpen={this.state.allTextShown}>
        Now we are engaged in a great civil war, testing whether that nation, or any nation so conceived and so dedicated, can long endure. We are met on a great battle-field of that war. We have come to dedicate a portion of that field, as a final resting place for those who here gave their lives that that nation might live. It is altogether fitting and proper that we should do this.
      <br />
      <br />
        But, in a larger sense, we can not dedicate -- we can not consecrate -- we can not hallow -- this ground. The brave men, living and dead, who struggled here, have consecrated it, far above our poor power to add or detract. The world will little note, nor long remember what we say here, but it can never forget what they did here. It is for us the living, rather, to be dedicated here to the unfinished work which they who fought here have thus far so nobly advanced. It is rather for us to be here dedicated to the great task remaining before us -- that from these honored dead we take increased devotion to that cause for which they gave the last full measure of devotion -- that we here highly resolve that these dead shall not have died in vain -- that this nation, under God, shall have a new birth of freedom -- and that government of the people, by the people, for the people, shall not perish from the earth.
      <br />
      <br />
      <Button onClick={this.handleShowLess} text="Show Less" style={buttonStyle} />
    </Toggle>
  </span>
</Alert>

This alert uses a Terra Toggle inside the body of the alert. Under the hood, the Toggle produces a <div> for its content. This is what the compiled HTML looks like:

<p class="Alert-module__section___IPbdu">
  <span>
    Four score and seven years ago our fathers brought forth on this continent, a new nation, conceived in Liberty, and
    dedicated to the proposition that all men are created equal.
    <div class="Toggle-module__toggle___1XQg0" aria-hidden="true">
    </div>
    <div class="Toggle-module__toggle___1XQg0" aria-hidden="false">Now we are engaged in a great civil war, testing
      whether that nation, or any nation so conceived and so dedicated, can long endure. We are met on a great
      battle-field of that war. We have come to dedicate a portion of that field, as a final resting place for those
      who here gave their lives that that nation might live. It is altogether fitting and proper that we should do
      this.<br><br>But, in a larger sense, we can not dedicate -- we can not consecrate -- we can not hallow -- this
      ground. The brave men, living and dead, who struggled here, have consecrated it, far above our poor power to add
      or detract. The world will little note, nor long remember what we say here, but it can never forget what they did
      here. It is for us the living, rather, to be dedicated here to the unfinished work which they who fought here
      have thus far so nobly advanced. It is rather for us to be here dedicated to the great task remaining before us
      -- that from these honored dead we take increased devotion to that cause for which they gave the last full
      measure of devotion -- that we here highly resolve that these dead shall not have died in vain -- that this
      nation, under God, shall have a new birth of freedom -- and that government of the people, by the people, for the
      people, shall not perish from the earth.<br><br><button class="Button-module__button___3afIE Button-module__neutral___ZyOh8"
        type="button" aria-disabled="false" style="text-decoration: underline;"><span class="Button-module__button-label___1dkzN Button-module__text-only___1GgY_">
          <span class="">Show Less
          </span>
        </span>
      </button>
    </div>
  </span>
</p>

As can be seen, a <div> is produced inside the <p> tag. Below is the code I have created to mimic this example:

return (<Alert
      onDismiss={this.handleDismiss}
      type="error"
    >
      {this.props.intl.formatMessage({ id: 'OrionWorklist.ErrorBannerDefaultMessage' })}
      <Spacer marginTop="medium">
        <Toggle isOpen={!this.state.errorBannerTextShown} isAnimated>
          <Button
            onClick={this.handleBannerButtonClick}
            text={this.props.intl.formatMessage({ id: 'OrionWorklist.ErrorBannerShowMoreButton' })}
          />
        </Toggle>
      </Spacer>
      <Toggle isOpen={this.state.errorBannerTextShown} isAnimated>
        <Spacer margin="large">
          <ul>
            {errorBannerBodyMessage.map(columnMessage =>
              (
                <li key={columnMessage}>
                  {columnMessage}
                </li>
              ),
            )}
          </ul>
        </Spacer>
        <Button
          onClick={this.handleBannerButtonClick}
          text={this.props.intl.formatMessage({ id: 'OrionWorklist.ErrorBannerShowLessButton' })}
        />
      </Toggle>
    </Alert>);

Here is the compiled HTML:

<p class="Alert-module__section___DDsAN">
  <strong class="Alert-module__title___4ksHs">Error.</strong>
  <span>Column(s) encountered an error
    <div class="Spacer-module__margin-top-medium___23OpH Spacer-module__margin-bottom-none___28Tnp Spacer-module__margin-left-none___36WWk Spacer-module__margin-right-none___33YSF Spacer-module__padding-top-none___2DwIm Spacer-module__padding-bottom-none___3pa1u Spacer-module__padding-left-none___2w6E4 Spacer-module__padding-right-none___q8G3k">
      <div class="Toggle-module__toggle___34f0E" aria-hidden="false">
        <div aria-hidden="false" class="rah-static rah-static--height-auto" style="height: auto;">
          <div style=""><button class="Button-module__button___3afIE Button-module__neutral___ZyOh8" type="button"
              aria-disabled="false"><span class="Button-module__button-label___1dkzN Button-module__text-only___1GgY_"><span
                  class="">More Details</span></span></button></div>
        </div>
      </div>
    </div>
    <div class="Toggle-module__toggle___34f0E" aria-hidden="true">
      <div aria-hidden="false" class="rah-static" style="height: 0px; overflow: hidden;">
        <div style="">
          <div class="Spacer-module__margin-top-large___1pOTg Spacer-module__margin-bottom-large___3rYjF Spacer-module__margin-left-large___3rJAt Spacer-module__margin-right-large___3F-eE Spacer-module__padding-top-none___2DwIm Spacer-module__padding-bottom-none___3pa1u Spacer-module__padding-left-none___2w6E4 Spacer-module__padding-right-none___q8G3k">
            <ul>
              <li>patient - column encountered an error <br></li>
              <li>location - column encountered an error <br></li>
              <li>primaryCareProvider - column encountered an error <br></li>
              <li>primaryContact - column encountered an error <br></li>
            </ul>
          </div><button class="Button-module__button___3afIE Button-module__neutral___ZyOh8" type="button"
            aria-disabled="false"><span class="Button-module__button-label___1dkzN Button-module__text-only___1GgY_"><span
                class="">Less Details</span></span></button>
        </div>
      </div>
    </div>
  </span>
</p>

As in the case above, the Terra Toggle also creates a <div> (I am also building a <ul> which the DOM validator also doesn't like) inside the alert's <p>. This is causing the error mentioned in the original issue comment. Below are screenshots the banner we are building and what we aim to achieve (preferably without the DOM validation errors if possible):

One more note I would like to point out, is that when I open the console in the production Terra Dev Site, where the above cited example is shown, there are no DOM nesting errors. I asked @tbiethman about this and he believes that this may be due to the fact that we are using the development build of React, not the production build.

Therefore, I believe the documentation is misleading as it states that HTML elements are valid for the children prop. To resolve this issue, I believe the documentation should be amended or the Terra Alert component should be modified to allow the use of any HTML elements inside its body.

[tbiethman]

I could see us updating the Alert to render children outside the <p> tag: https://github.com/cerner/terra-core/blob/master/packages/terra-alert/src/Alert.jsx#L166

  // Current
  const alertMessageContent = (
    <p className={alertSectionClassName}>
      {(title || defaultTitle) && <strong className={cx('title')}>{title || defaultTitle}</strong>}
      {children}
    </p>
  );

  // New
  const alertMessageContent = (
    <React.Fragment>
      <p className={alertSectionClassName}>
        {(title || defaultTitle) && <strong className={cx('title')}>{title || defaultTitle}</strong>}
      </p>
      {children}
    </React.Fragment>
  );

The problem with that is we're currently using children to provide the rest of the alert string to append to the title. If we do want to support use cases like this (and though there is an example with it, I'm not entirely sure that we do @neilpfeiffer), to remain passive we would need to create a new prop for this additional content.

[JakeLaCombe]

I did a screen reader test with Jaws and Voiceover using a div instead of a paragraph. We wanted to make sure that the title and message read as a seamless statement to browsers, and it used to break on the title with a pause before reading the alert. It must have been a style change, not a markup that changed that, so I would be okay with using a div instead of a p for alert. I do see this as a valid case for an alert banner, but I would like to double check with our designer.

[noahbenham]

Any word back from your designer @JakeLaCombe?

[JakeLaCombe]

Still waiting for that feedback

[bjankord]

Talked with @neilpfeiffer and there are weren't any concerns noted about using a div for this.

[neilpfeiffer]

Since there isn't an accessibility imperative to keep this a <p> element, and that after testing it seems to be non-affecting change, using a <div> should be completely fine to allow for more complex context in special situations within the content area of the banner.

Special Note -- a future major enhancement to this component will have the show/hide feature built in and so you may want to update from your current implementation @andrewnottaviano to the new API once available. That work has not been planned yet, so unclear when it will be worked on, but it is captured and backlogged with issue #1882

[gabeparra01]

@andrewnottaviano @tbiethman @JakeLaCombe I can work on this if no one has picked it up yet

[JakeLaCombe]

@gabeparra01 you are welcome to contribute this! It would be appreciated!

[gabeparra01]

Ok I will start on it now