-
Notifications
You must be signed in to change notification settings - Fork 17
fix(Versions): replace radio with sections #2773
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
| <Button size="s"> | ||
| {i18n('nodes-count', {count: nodesAmount})} | ||
| <Icon data={ArrowRight} /> | ||
| </Button> |
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.
Button has href prop, but href makes page reload. Used button to align with design, but wrapped it with InternalLink to make spa-link
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.
Let's better use onClick property for Button.
button inside a is a bad practice: https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element (see [Content model](https://html.spec.whatwg.org/multipage/dom.html#concept-element-content-model) block)
To speed up, let's merge this PR as is and fix in the next.
|
bugbot run |
|
Claude finished @artemmufazalov's task —— View job Code Review for PR #2773: fix(Versions): replace radio with sectionsI've completed my review of the changes in this PR. Here's my analysis: Review Checklist
SummaryThe PR successfully replaces radio button tabs with collapsible sections in the Versions page, improving the UI organization and user experience. The implementation is clean and follows project conventions well. ✅ Positive Aspects
🔍 Areas for Improvement
📋 Recommendations
✅ Security & Best Practices
ConclusionThe PR successfully achieves its goals and follows most project conventions. The code is clean and maintainable. With the minor performance optimizations suggested above, this would be ready for merge. |
599c55d to
189aa30
Compare
|
bugbot run |
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.
✅ Bugbot reviewed your changes and found no bugs!
| <Button size="s"> | ||
| {i18n('nodes-count', {count: nodesAmount})} | ||
| <Icon data={ArrowRight} /> | ||
| </Button> |
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.
Let's better use onClick property for Button.
button inside a is a bad practice: https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element (see [Content model](https://html.spec.whatwg.org/multipage/dom.html#concept-element-content-model) block)
To speed up, let's merge this PR as is and fix in the next.
Closes #2772
Stand: https://nda.ya.ru/t/OV4kCgL-7JC3nR
Before:
After:
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: ✅
Current: 85.37 MB | Main: 85.38 MB
Diff: 2.68 KB (-0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information