chore(Alert): Allowed for non header elements to be set at the Alert title #8518
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Preview: https://patternfly-react-pr-8518.surge.sh A11y report: https://patternfly-react-pr-8518-a11y.surge.sh |
| @@ -50,7 +50,9 @@ PatternFly supports several properties and variations that can be used to add ex | |||
|
|
|||
| * As demonstrated in the 3rd and 4th variations below, use the `actionClose` property to add an `<AlertActionCloseButton>` component, which can be used to manage and customize alert dismissals. `actionClose` can be used with or without the presence of `actionLinks`. | |||
|
|
|||
| * As demonstrated in the 5th and 6th variations below, use the `titleHeadingLevel` property to set the heading level of an alert title. Headings should be ordered by their level and heading levels should not be skipped. For example, a heading of an `h2` level should not be followed directly by an `h4`. | |||
| * As demonstrated in the 5th and 6th variations below, use the `component` property to set the element for an alert title. | |||
| * If there is no description below the alert title, then the `component` prop could be set to a non-heading element such as a `span` or `div`. | |||
There was a problem hiding this comment.
Nit, just to make it more of a recommendation:
Suggested change
| * If there is no description below the alert title, then the `component` prop could be set to a non-heading element such as a `span` or `div`. | |
| * If the `description` prop is not passed in, then the `component` prop should be set to a non-heading element such as a `span` or `div`. |
We should also open up a followup issue for org to update the a11y docs
There was a problem hiding this comment.
ah yeah - those are old, but I can take a look at seeing about cleaning it up |
| @@ -175,7 +186,7 @@ export const Alert: React.FunctionComponent<AlertProps> = ({ | |||
| }, [containsFocus, isMouseOver]); | |||
| React.useEffect(() => { | |||
| dismissed && onTimeout(); | |||
| }, [dismissed]); | |||
| }, [dismissed, timeoutAnimation]); | |||
There was a problem hiding this comment.
Looks like this was added to the wrong useEffect. onTimeout could be added here to dismiss the GH warning and timeoutAnimation added to the useEffect above this one.
Comment on lines
159
to
164
| const calculatedTimeout = timeout === true ? 8000 : Number(timeout); | ||
| if (calculatedTimeout > 0) { | ||
| const timer = setTimeout(() => setTimedOut(true), calculatedTimeout); | ||
| return () => clearTimeout(timer); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
GH is still warning about missing timeout in the dependency array here
thatblindgeye
approved these changes
Jan 13, 2023
kmcfaul
approved these changes
Jan 13, 2023
|
Just needs a rebase and we can merge. |
nicolethoen
force-pushed
the
alert_custom_component
branch
from
942f8ec
to
fa4ba73
Compare
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What: Closes #8402
deprecates the
titleHeadingLeveladds a
componentpropupdates existing examples on the Alert page to avoid new console warnings and advise the use of the component prop.
Created new issue to track the removal of the deprecated titleHeadingLevel prop: #8519