Skip to content

feat(Compass): add compass nav components #12138

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
merged 19 commits into from
Nov 19, 2025
Merged

feat(Compass): add compass nav components #12138

merged 19 commits into from
Nov 19, 2025

Conversation

@wise-king-sullyman
Copy link
Contributor

@wise-king-sullyman wise-king-sullyman commented Nov 13, 2025 •
edited
Loading

@patternfly-build
Copy link
Contributor

patternfly-build commented Nov 13, 2025 •
edited
Loading

Preview: https://pf-react-pr-12138.surge.sh

A11y report: https://pf-react-pr-12138-a11y.surge.sh

srambach
srambach reviewed Nov 17, 2025
View reviewed changes
...ges/react-core/src/components/Compass/__tests__/__snapshots__/CompassNavSearch.test.tsx.snap Outdated
Comment on lines 8 to 10
<div
style="display: contents;"
>
Copy link
Member

@srambach srambach Nov 17, 2025

Choose a reason for hiding this comment

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

Is it expected to get this extra div around the search contents? It's here and on the nav-home also

Copy link
Contributor Author

@wise-king-sullyman wise-king-sullyman Nov 17, 2025

Choose a reason for hiding this comment

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

Yeah that's coming from the Tooltip

Copy link
Member

@srambach srambach Nov 17, 2025

Choose a reason for hiding this comment

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

Cool, looks good then!

Copy link
Contributor

@kmcfaul kmcfaul Nov 17, 2025

Choose a reason for hiding this comment

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

Do we want to update the internal tooltips to use triggerRef to avoid this styled wrapping div? I know we've tried to avoid that div in most places but it was breaking to do that for Tooltip at the time iirc.

Copy link
Contributor Author

@wise-king-sullyman wise-king-sullyman Nov 17, 2025

Choose a reason for hiding this comment

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

I'm not opposed to that

srambach
srambach approved these changes Nov 17, 2025
View reviewed changes
...ges/react-core/src/components/Compass/__tests__/__snapshots__/CompassNavSearch.test.tsx.snap Outdated
Comment on lines 8 to 10
<div
style="display: contents;"
>
Copy link
Member

@srambach srambach Nov 17, 2025

Choose a reason for hiding this comment

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

Cool, looks good then!

kmcfaul
kmcfaul requested changes Nov 17, 2025
View reviewed changes
Copy link
Contributor

@kmcfaul kmcfaul left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a few things to fix up the props tables and a question

...ges/react-core/src/components/Compass/__tests__/__snapshots__/CompassNavSearch.test.tsx.snap Outdated
Comment on lines 8 to 10
<div
style="display: contents;"
>
Copy link
Contributor

@kmcfaul kmcfaul Nov 17, 2025

Choose a reason for hiding this comment

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

Do we want to update the internal tooltips to use triggerRef to avoid this styled wrapping div? I know we've tried to avoid that div in most places but it was breaking to do that for Tooltip at the time iirc.

wise-king-sullyman and others added 6 commits November 17, 2025 14:30
@wise-king-sullyman @kmcfaul
Update packages/react-core/src/components/Compass/CompassNavContent.tsx
99d749b 
Co-authored-by: kmcfaul <45077788+kmcfaul@users.noreply.github.com>
@wise-king-sullyman @kmcfaul
Update packages/react-core/src/components/Compass/CompassNavHome.tsx
f7c0e43 
Co-authored-by: kmcfaul <45077788+kmcfaul@users.noreply.github.com>
@wise-king-sullyman @kmcfaul
Update packages/react-core/src/components/Compass/CompassNavMain.tsx
e9539a1 
Co-authored-by: kmcfaul <45077788+kmcfaul@users.noreply.github.com>
@wise-king-sullyman @kmcfaul
Update packages/react-core/src/components/Compass/CompassNavSearch.tsx
54ed2b9 
Co-authored-by: kmcfaul <45077788+kmcfaul@users.noreply.github.com>
@wise-king-sullyman
format
d454281
@wise-king-sullyman
Fix dep issue introduced during merge conflict resolution
edc6b88
thatblindgeye
thatblindgeye requested changes Nov 18, 2025
View reviewed changes
Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Really just a nit, but could we have the Home tooltip default to the left, but update when necessary?

@wise-king-sullyman @thatblindgeye
Update packages/react-core/src/components/Compass/CompassNavContent.tsx
4c8852d 
Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>
@kmcfaul kmcfaul merged commit 4bd98bc into patternfly:main Nov 19, 2025
13 checks passed
@patternfly-build
Copy link
Contributor

patternfly-build commented Nov 19, 2025

Your changes have been released in:

  • @patternfly/react-code-editor@6.5.0-prerelease.15
  • @patternfly/react-core@6.5.0-prerelease.15
  • @patternfly/react-docs@7.5.0-prerelease.16
  • @patternfly/react-drag-drop@6.5.0-prerelease.15
  • demo-app-ts@6.5.0-prerelease.15
  • @patternfly/react-table@6.5.0-prerelease.15
  • @patternfly/react-templates@6.5.0-prerelease.15

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srambach srambach srambach approved these changes

@kmcfaul kmcfaul kmcfaul approved these changes

@thatblindgeye thatblindgeye thatblindgeye approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Tabs/Nav - structure needs to support more than just nav items

5 participants

@wise-king-sullyman @patternfly-build @srambach @kmcfaul @thatblindgeye