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

fix(AlertActionLink) support ReactNode as child #9278

Merged

Conversation

Dominik-Petrik
Copy link
Contributor

What: Closes #9006

@patternfly-build
Copy link
Contributor

patternfly-build commented Jun 16, 2023

Copy link
Contributor

@gitdallas gitdallas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - should a test/demo be added?

Comment on lines 23 to 33
test('Renders ReactNode as children', () => {
render(
<AlertActionLink>
<a href="https://test.link.com/" target="_blank" rel="noreferrer" title="Learn more link">
Learn more
</a>
</AlertActionLink>
);

expect(screen.getByRole('link')).toBeVisible();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it's just a test, I feel like it'd be better to test a structure we'd recommend/is valid. Nesting interactive content like a link inside a button isn't valid and will throw an a11y error in Axe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I used the code snippet provided by author of the issue for the test.

Comment on lines 9 to 10
/** Content rendered inside the alert action link. */
children?: string;
children?: string | React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the above, we could update the description here just to offer a warning, "Content rendered inside the alert action link. You should not pass in interactive content such as anchor elements." I'll also open a followup in Org to update the a11y docs to mention this.

We should be able to just type this as children?: React.ReactNode as well since that'll include the string type, though we do have some other components that type a prop as string | React.ReactNode (@tlabaj I imagine we could update those instances as a followup?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thatblindgeye that is correct React.ReactNode includes string so string ins not needed here. We can open a up a follow up issue to clean up other interface that have the redundant definition.

@Dominik-Petrik Dominik-Petrik force-pushed the alert-actionLink-allowNodeChildren branch from 9378b91 to 184e4ca Compare July 11, 2023 10:51
@tlabaj tlabaj changed the base branch from main to postV5 July 26, 2023 20:08
@tlabaj tlabaj merged commit 7b33179 into patternfly:postV5 Jul 26, 2023
tlabaj pushed a commit to tlabaj/patternfly-react that referenced this pull request Jul 31, 2023
* fix(AlertActionLink) support ReactNode as child

* add test

* update prop type and description

* update test
wise-king-sullyman pushed a commit that referenced this pull request Jul 31, 2023
* fix(Toolbar): resolved typeerror on full page demo (#9355)

* chore(TreeView): converted examples to TS (#9286)

* fix(ExpandableSection): added ARIA attributes (#9303)

* fix(ExpandableSection): added ARIA attributes

* Updated failing snapshots due to mismatching generated ID

* chore(Tooltip): updated unit tests (#9295)

* chore(Tooltip): updated unit tests

* Updated mock and tests

* Updated based on Austin feedback

* Updated integration tests

* Removed unused imports

* Updated remaining tests using Popper mock

* Removed extraenous snapshot test

* Removed test

* Split out onTooltipHidden test

* chore(Card): added tests for new clickable/selectable (#9262)

* chore(Card): added tests for new clickable/selectable

* Added tests for clickable cards

* Updated card with actions test

* fix(Slider): reverted taborder (#9293)

* fix(chore): Fix deprecated wizard integration tests (#9312)

* fix(chore): Fix deprecated wizard integration tests

* updated non deprecated test as well

---------

Co-authored-by: Titani <tlabaj@redaht.com>

* Rebase postv5 (#9393)

* chore(deps): bump docs framework (#9370)

* chore(docs): Updated screenshots (#9337)

* chore(docs): Updated screenshots

* updated screenshots after logo update

---------

Co-authored-by: Titani <tlabaj@redaht.com>

* chore(release): releasing packages [ci skip]

 - @patternfly/react-docs@6.0.0-prerelease.26

* chore(deps): bump to latest chore version (#9389)

* chore(deps): bump to latest chore version

* bump to 16

* chore(release): releasing packages [ci skip]

 - @patternfly/react-charts@7.0.0-prerelease.12
 - @patternfly/react-code-editor@5.0.0-prerelease.24
 - @patternfly/react-core@5.0.0-prerelease.24
 - @patternfly/react-docs@6.0.0-prerelease.27
 - @patternfly/react-icons@5.0.0-prerelease.9
 - demo-app-ts@5.0.0-prerelease.22
 - @patternfly/react-styles@5.0.0-prerelease.7
 - @patternfly/react-table@5.0.0-prerelease.24
 - @patternfly/react-tokens@5.0.0-prerelease.9

* fix(fileupload): use default readonly text input instead of plain (#9387)

* fix(fileupload): use default readonly text input instead of plain

* chore(build): snaps

* fix(CodeEditor): prevent clicks in textarea from opening fileupload (#9385)

* fix(toolbar): added chip container class to toolbar content (#9379)

* feat(Menu): added support for tooltips to menu (#9382)

* fix(whitespace): Update readme to trigger release

* chore(release): releasing packages [ci skip]

 - @patternfly/react-code-editor@5.0.0-prerelease.25
 - @patternfly/react-core@5.0.0-prerelease.25
 - @patternfly/react-docs@6.0.0-prerelease.28
 - demo-app-ts@5.0.0-prerelease.23
 - @patternfly/react-table@5.0.0-prerelease.25

* fix(Toolbar): resolved typeerror on full page demo (#9355)

* chore(TreeView): converted examples to TS (#9286)

* fix(ExpandableSection): added ARIA attributes (#9303)

* fix(ExpandableSection): added ARIA attributes

* Updated failing snapshots due to mismatching generated ID

* chore(Tooltip): updated unit tests (#9295)

* chore(Tooltip): updated unit tests

* Updated mock and tests

* Updated based on Austin feedback

* Updated integration tests

* Removed unused imports

* Updated remaining tests using Popper mock

* Removed extraenous snapshot test

* Removed test

* Split out onTooltipHidden test

* chore(Card): added tests for new clickable/selectable (#9262)

* chore(Card): added tests for new clickable/selectable

* Added tests for clickable cards

* Updated card with actions test

* fix(Slider): reverted taborder (#9293)

* fix(chore): Fix deprecated wizard integration tests (#9312)

* fix(chore): Fix deprecated wizard integration tests

* updated non deprecated test as well

---------

Co-authored-by: Titani <tlabaj@redaht.com>

---------

Co-authored-by: Titani <tlabaj@redaht.com>
Co-authored-by: patternfly-build <patternfly-build@redhat.com>
Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com>
Co-authored-by: Dallas <dallas.nicol@gmail.com>
Co-authored-by: Dana Gutride <dgutride@gmail.com>
Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>

* feat(MenuItem): allow target and rel on links (#9294)

* feat(MenuItem): allow target and rel on links

* update desc

* move desc to right prop

* chore(deps): update dependency lint-staged to v13.2.3 (#9335)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(JumpLinks) href does not work properly (#9307)

* chore(deps): update dependency eslint to v8.42.0 (#9236)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(SelectToggle): corrected removeEventListener on unmount for v5 (#9380)

* fix(Page) allow change of main tag (#9296)

* fix(Page) allow change of main tag

* rename prop and add it to render method

* change prop name and description

* update description

* Reverted prop name

---------

Co-authored-by: Eric Olkowski <thatblindgeye@gmail.com>

* fix(AlertActionLink) support ReactNode as child (#9278)

* fix(AlertActionLink) support ReactNode as child

* add test

* update prop type and description

* update test

* fix: changed the button lable from Upload to Browse (#9275)

* fix: changed the button lable from Upload to Browse

* change button label from hardcoded text to props element

* passes label as Browse

* Update MultipleFileUploadMain.tsx

* made changes as per your requirements

* follow-up

* follow-up

* follow-up

* fix snaphots

---------

Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>
Co-authored-by: Titani <tlabaj@redaht.com>
Co-authored-by: patternfly-build <patternfly-build@redhat.com>
Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com>
Co-authored-by: Dallas <dallas.nicol@gmail.com>
Co-authored-by: Dana Gutride <dgutride@gmail.com>
Co-authored-by: kmcfaul <45077788+kmcfaul@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Dominik Petřík <77832970+Dominik-Petrik@users.noreply.github.com>
Co-authored-by: Eric Olkowski <thatblindgeye@gmail.com>
Co-authored-by: Tarun Samanta <tarunsamanta77@gmail.com>
nicolethoen pushed a commit to Kells512/patternfly-react that referenced this pull request Sep 1, 2023
* fix(Toolbar): resolved typeerror on full page demo (patternfly#9355)

* chore(TreeView): converted examples to TS (patternfly#9286)

* fix(ExpandableSection): added ARIA attributes (patternfly#9303)

* fix(ExpandableSection): added ARIA attributes

* Updated failing snapshots due to mismatching generated ID

* chore(Tooltip): updated unit tests (patternfly#9295)

* chore(Tooltip): updated unit tests

* Updated mock and tests

* Updated based on Austin feedback

* Updated integration tests

* Removed unused imports

* Updated remaining tests using Popper mock

* Removed extraenous snapshot test

* Removed test

* Split out onTooltipHidden test

* chore(Card): added tests for new clickable/selectable (patternfly#9262)

* chore(Card): added tests for new clickable/selectable

* Added tests for clickable cards

* Updated card with actions test

* fix(Slider): reverted taborder (patternfly#9293)

* fix(chore): Fix deprecated wizard integration tests (patternfly#9312)

* fix(chore): Fix deprecated wizard integration tests

* updated non deprecated test as well

---------

Co-authored-by: Titani <tlabaj@redaht.com>

* Rebase postv5 (patternfly#9393)

* chore(deps): bump docs framework (patternfly#9370)

* chore(docs): Updated screenshots (patternfly#9337)

* chore(docs): Updated screenshots

* updated screenshots after logo update

---------

Co-authored-by: Titani <tlabaj@redaht.com>

* chore(release): releasing packages [ci skip]

 - @patternfly/react-docs@6.0.0-prerelease.26

* chore(deps): bump to latest chore version (patternfly#9389)

* chore(deps): bump to latest chore version

* bump to 16

* chore(release): releasing packages [ci skip]

 - @patternfly/react-charts@7.0.0-prerelease.12
 - @patternfly/react-code-editor@5.0.0-prerelease.24
 - @patternfly/react-core@5.0.0-prerelease.24
 - @patternfly/react-docs@6.0.0-prerelease.27
 - @patternfly/react-icons@5.0.0-prerelease.9
 - demo-app-ts@5.0.0-prerelease.22
 - @patternfly/react-styles@5.0.0-prerelease.7
 - @patternfly/react-table@5.0.0-prerelease.24
 - @patternfly/react-tokens@5.0.0-prerelease.9

* fix(fileupload): use default readonly text input instead of plain (patternfly#9387)

* fix(fileupload): use default readonly text input instead of plain

* chore(build): snaps

* fix(CodeEditor): prevent clicks in textarea from opening fileupload (patternfly#9385)

* fix(toolbar): added chip container class to toolbar content (patternfly#9379)

* feat(Menu): added support for tooltips to menu (patternfly#9382)

* fix(whitespace): Update readme to trigger release

* chore(release): releasing packages [ci skip]

 - @patternfly/react-code-editor@5.0.0-prerelease.25
 - @patternfly/react-core@5.0.0-prerelease.25
 - @patternfly/react-docs@6.0.0-prerelease.28
 - demo-app-ts@5.0.0-prerelease.23
 - @patternfly/react-table@5.0.0-prerelease.25

* fix(Toolbar): resolved typeerror on full page demo (patternfly#9355)

* chore(TreeView): converted examples to TS (patternfly#9286)

* fix(ExpandableSection): added ARIA attributes (patternfly#9303)

* fix(ExpandableSection): added ARIA attributes

* Updated failing snapshots due to mismatching generated ID

* chore(Tooltip): updated unit tests (patternfly#9295)

* chore(Tooltip): updated unit tests

* Updated mock and tests

* Updated based on Austin feedback

* Updated integration tests

* Removed unused imports

* Updated remaining tests using Popper mock

* Removed extraenous snapshot test

* Removed test

* Split out onTooltipHidden test

* chore(Card): added tests for new clickable/selectable (patternfly#9262)

* chore(Card): added tests for new clickable/selectable

* Added tests for clickable cards

* Updated card with actions test

* fix(Slider): reverted taborder (patternfly#9293)

* fix(chore): Fix deprecated wizard integration tests (patternfly#9312)

* fix(chore): Fix deprecated wizard integration tests

* updated non deprecated test as well

---------

Co-authored-by: Titani <tlabaj@redaht.com>

---------

Co-authored-by: Titani <tlabaj@redaht.com>
Co-authored-by: patternfly-build <patternfly-build@redhat.com>
Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com>
Co-authored-by: Dallas <dallas.nicol@gmail.com>
Co-authored-by: Dana Gutride <dgutride@gmail.com>
Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>

* feat(MenuItem): allow target and rel on links (patternfly#9294)

* feat(MenuItem): allow target and rel on links

* update desc

* move desc to right prop

* chore(deps): update dependency lint-staged to v13.2.3 (patternfly#9335)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(JumpLinks) href does not work properly (patternfly#9307)

* chore(deps): update dependency eslint to v8.42.0 (patternfly#9236)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(SelectToggle): corrected removeEventListener on unmount for v5 (patternfly#9380)

* fix(Page) allow change of main tag (patternfly#9296)

* fix(Page) allow change of main tag

* rename prop and add it to render method

* change prop name and description

* update description

* Reverted prop name

---------

Co-authored-by: Eric Olkowski <thatblindgeye@gmail.com>

* fix(AlertActionLink) support ReactNode as child (patternfly#9278)

* fix(AlertActionLink) support ReactNode as child

* add test

* update prop type and description

* update test

* fix: changed the button lable from Upload to Browse (patternfly#9275)

* fix: changed the button lable from Upload to Browse

* change button label from hardcoded text to props element

* passes label as Browse

* Update MultipleFileUploadMain.tsx

* made changes as per your requirements

* follow-up

* follow-up

* follow-up

* fix snaphots

---------

Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>
Co-authored-by: Titani <tlabaj@redaht.com>
Co-authored-by: patternfly-build <patternfly-build@redhat.com>
Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com>
Co-authored-by: Dallas <dallas.nicol@gmail.com>
Co-authored-by: Dana Gutride <dgutride@gmail.com>
Co-authored-by: kmcfaul <45077788+kmcfaul@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Dominik Petřík <77832970+Dominik-Petrik@users.noreply.github.com>
Co-authored-by: Eric Olkowski <thatblindgeye@gmail.com>
Co-authored-by: Tarun Samanta <tarunsamanta77@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug - <AlertActionLink> - does not support ReactNode as its child
6 participants