-
Notifications
You must be signed in to change notification settings - Fork 33
Enable CMD/CTRL-click behavior for sidebar navigation (#174) #233
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
Conversation
Signed-off-by: Andrew Sawers <[email protected]>
Signed-off-by: Andrew Sawers <[email protected]>
2dcac95 to
874a139
Compare
|
DCO is fixed and this should now be ready for review. It’s my first PR in a while, so I’m very happy to make any changes or follow any preferred conventions. |
EnriqueL8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing before I dive any deeper
| @@ -1,30 +1,13 @@ | |||
| // Copyright © 2022 Kaleido, Inc. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to keep this copyright - it stays from the original contributor but you can add the org you belong along side it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, updated now. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @standrew for this PR - looks good
On external links, I think since we are already doing the work it makes sense to fix it for any link we believe needs to be external - not the ones that open in right hand side modal for example
I realize we don't have a proper testing framework here, this might be something to look into contributing
Build falling
Failed to compile.
src/components/Navigation/Navigation.tsx
Line 26:9: 'navigate' is assigned a value but never used @typescript-eslint/no-unused-vars
| isRoot?: boolean; | ||
| } | ||
|
|
||
| // NEW: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment removed. :-)
src/interfaces/navigation.ts
Outdated
| // NEW optional props for link-style behavior | ||
| to?: string; // internal React Router path | ||
| href?: string; // external URL | ||
|
|
||
| // fallback for legacy click handlers | ||
| action?: () => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather reverse this with deprecated for the action?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected. :-)
|
I have pushed the updates based on the review feedback. Breadcrumb ChangesThe breadcrumb component has been updated to use proper link semantics. File updated: src/components/Breadcrumbs/FFBreadcrumb.tsx Navigation Feedback ResolvedAll navigation-related feedback from the review has been addressed. These changes were committed separately to keep the history clear. Files updated:
Regarding Broader Link / URL RefactoringOther areas of the UI still use imperative navigation (navigate, slide, etc.) because they depend on the existing route structure in navigation.ts. For this PR, the scope has been intentionally limited to:
If desired, I can follow up with a separate issue to standardise link behaviour more broadly and also add testing (Playwright/Cypress?). Potential Future Candidates for Link ConversionBelow is a non-exhaustive list of components and areas currently using imperative navigation that could be refactored in a future update: Buttons and Launchers
Cards, Accordions, and Lists
Enum-Driven UI Launchers
These areas are more interconnected, so switching them to link semantics would require a coordinated refactor. Let me know if any additional changes are needed or if a broader follow-up refactor would be helpful. |
Signed-off-by: Andrew Sawers <[email protected]>
Signed-off-by: Andrew Sawers <[email protected]>
c91f4c1 to
ac200c5
Compare
|
Awesome thank @ajmsawers , I plan to run this locally and try it out before getting it in |
✔ What’s done
Refactored sidebar navigation items to use React Router
<Link>behavior via NavItem with newto/hrefprops.All sidebar navigation entries now behave like real links:
❓ Looking for clarification
Since I’m still new to the codebase, I want to confirm whether any non-sidebar UI elements should also support CMD/CTRL + click.
For example, the “external-link” style icon buttons (like the purple circular icon and small external-link icon in the timeline area) sometimes link to another page or open content in a modal.
Before updating those, I want to make sure they’re intended to behave as actual links rather than action buttons.
If these icon buttons should also follow standard link semantics, I can update them to use
<Link>/<a>so they support CMD/CTRL + click as well.Let me know if I missed any others. 🙂
Partially addresses #174 — this PR handles sidebar navigation CMD/CTRL-click behavior. Awaiting clarification on non-sidebar links before completing the rest.