-
Notifications
You must be signed in to change notification settings - Fork 300
Fixes #37256 - Remove href from labels to prevent warnings #11398
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
base: master
Are you sure you want to change the base?
Conversation
Reviewer's GuideHost count labels are converted from anchor elements with href to button elements, eliminating unintended navigation and React warnings while preserving modal click functionality. Sequence Diagram: Label Click to Modal DisplaysequenceDiagram
actor User
participant LabelButton as "Label (as button)"
participant ComponentLogic as "UI Component Logic"
participant Modal
User->>LabelButton: Clicks Label
LabelButton->>ComponentLogic: Executes onClick handler (e.g., setModalState(true))
ComponentLogic->>Modal: Triggers Modal display
Modal-->>User: Shows Modal content
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @pavanshekar - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -26,9 +26,9 @@ const Hosts = ({ | |||
{ __('Some hosts are not registered as content hosts and will be ignored.') } | |||
</p> | |||
} | |||
{contentHosts.length > 0 && (<Label color="green" href="#" onClick={() => setModalHosts(true)}>{titleHosts}: {contentHosts.length}</Label>)} |
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.
issue (bug_risk): Removing href may break keyboard accessibility
To restore keyboard focus, render the Label as a native button (e.g. <Label as="button" …>) or add role="button" and tabIndex={0}.
@sourcery-ai review |
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.
Hey @pavanshekar - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Are there other pages in the UI with similar console warnings? I remember seeing this when navigating to some pages on the CV UI.
A similar warning is also present in the RoutedTabs component on the Content View Details page. This warning occurs because state updates or navigation are being triggered during the render phase of the component, which conflicts with React’s lifecycle. This happens as the navigation logic inside RoutedTabs tries to update the location or active state while React is still rendering, leading to the warning. |
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.
LGTM and works well
What are the changes introduced in this pull request?
Removed the href attributes from the Label components, so they no longer render as anchors, and relies solely on their onClick handlers to open the modals. This stops any unintended navigation and resolves the React “cannot update during render” warnings.
Considerations taken when implementing this change?
Confirmed that click behavior and modal logic remain fully functional across supported browsers and that no new console errors appear.
What are the testing steps for this pull request?
Summary by Sourcery
Bug Fixes: