Skip to content

fix(app-launcher): less aggressive router linking #3995

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.

Sign up for GitHub

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

Jump to bottom
Merged
redallen merged 6 commits into from
Mar 31, 2020
Merged

fix(app-launcher): less aggressive router linking #3995

redallen merged 6 commits into from
Mar 31, 2020

Conversation

@redallen
Copy link
Contributor

@redallen redallen commented Mar 30, 2020 •
edited
Loading

What: Fixes broken application launcher styles on master by choosing to support this API:

component={<Link to="/components/alert/">Alert</Link>}
OR
component="button"

rather than

<DropdownItem component="some-custom-string-that-says-just-to-render-children">
  <Link to="/components/alert/">Alert</Link>
</DropdownItem>
OR
component={<Link to="/components/alert/">Alert</Link>}
OR
component="button"

Changes to InternalDropdownItem (affects Dropdown, OptionsMenu, and ApplicationLauncher):

  • If component is a valid element, clone it rather than children
  • Pass href and id to cloned element

@patternfly-build
Copy link
Collaborator

patternfly-build commented Mar 30, 2020 •
edited
Loading

PF4 preview: https://patternfly-react-pr-3995.surge.sh
A11y report: https://patternfly-react-pr-3995-coverage.surge.sh

@redallen redallen requested review from mcoker and tlabaj March 30, 2020 20:07
@tlabaj tlabaj requested review from jschuler and mcoker and removed request for mcoker and tlabaj March 30, 2020 20:09
mcoker
mcoker reviewed Mar 30, 2020
View reviewed changes
packages/react-core/src/components/ApplicationLauncher/examples/ApplicationLauncher.md
<Text component="a" href="#" style={exampleStyle}>
Router link
</Text>
<Link to="/" style={exampleStyle}>
Copy link
Contributor

@mcoker mcoker Mar 30, 2020

Choose a reason for hiding this comment

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

is style={exampleStyle} necessary for the example?

tlabaj
tlabaj approved these changes Mar 30, 2020
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@redallen redallen merged commit 28d653e into patternfly:master Mar 31, 2020
@patternfly-build
Copy link
Collaborator

patternfly-build commented Mar 31, 2020

Your changes have been released in:

  • @patternfly/react-catalog-view-extension@1.4.47
  • @patternfly/react-core@3.153.2
  • @patternfly/react-docs@4.21.6
  • @patternfly/react-inline-edit-extension@2.17.47
  • demo-app-ts@3.30.2
  • @patternfly/react-table@2.28.28
  • @patternfly/react-topology@2.14.47
  • @patternfly/react-virtualized-extension@1.4.48

Thanks for your contribution! 🎉

This was referenced Apr 2, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcoker mcoker mcoker approved these changes

@tlabaj tlabaj tlabaj approved these changes

@jschuler jschuler Awaiting requested review from jschuler

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@redallen @patternfly-build @jschuler @mcoker @tlabaj