Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request adds visual variant and size support to the Tabs component. Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/www/src/content/docs/components/tabs/demo.ts (1)
6-27:⚠️ Potential issue | 🟠 MajorPreview demo has mismatched
defaultValueand trigger values.The preview sets
defaultValue="tab-one"but noTabs.Tabexists fortab-one, and it also includes content entries without corresponding triggers.💡 Suggested fix
- <Tabs defaultValue="tab-one"> + <Tabs defaultValue="tab-two"> @@ - <Tabs.Content value="tab-one"> - <Text>General settings content</Text> - </Tabs.Content> <Tabs.Content value="tab-two"> <Text>Hosting configuration content</Text> </Tabs.Content> - <Tabs.Content value="tab-three"> - <Text>Editor preferences content</Text> - </Tabs.Content> <Tabs.Content value="tab-four"> <Text>Billing information content</Text> </Tabs.Content>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/content/docs/components/tabs/demo.ts` around lines 6 - 27, The Tabs demo has mismatched values: the Tabs component sets defaultValue="tab-one" but there is no Tabs.Tab with value "tab-one", and several Tabs.Content entries (e.g., values "tab-one" and "tab-three") lack corresponding Tabs.Tab triggers; update the Tabs.List to include Tabs.Tab elements matching every Tabs.Content value or remove/rename the orphaned Tabs.Content values to match existing Tabs.Tab triggers (ensure defaultValue matches an existing Tabs.Tab value), referencing the Tabs component, its defaultValue prop, Tabs.List / Tabs.Tab entries, and Tabs.Content value attributes when making the changes.
🧹 Nitpick comments (2)
packages/raystack/components/tabs/__tests__/tabs.test.tsx (1)
321-353: Add asize="medium"assertion to complete size coverage.
mediumis part of the new public API but is not validated in this suite.💡 Suggested test addition
describe('Sizes', () => { @@ it('applies small size class', () => { @@ expect(root).toHaveClass(styles['size-small']); }); + + it('applies medium size class', () => { + render( + <Tabs defaultValue='tab1' size='medium'> + <Tabs.List> + <Tabs.Tab value='tab1'>{TAB_1_TEXT}</Tabs.Tab> + </Tabs.List> + <Tabs.Content value='tab1'>{CONTENT_1_TEXT}</Tabs.Content> + </Tabs> + ); + + const tablist = screen.getByRole('tablist'); + const root = tablist.closest(`.${styles.root}`); + expect(root).not.toBeNull(); + expect(root).toHaveClass(styles['size-medium']); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/tabs/__tests__/tabs.test.tsx` around lines 321 - 353, Add a new test in the "Sizes" suite that renders <Tabs defaultValue='tab1' size='medium'> with a Tabs.List/Tabs.Tab and Tabs.Content (same pattern as the existing tests), query the tablist via screen.getByRole('tablist'), find its closest element with styles.root, assert it's not null, and assert it has the class styles['size-medium'] to validate the medium size option for the Tabs component.packages/raystack/components/tabs/tabs.module.css (1)
77-79: Disabled trigger should not keep pointer cursor.Disabled tabs still look clickable because
.triggersetscursor: pointerand the disabled override only changes opacity.💡 Suggested fix
.trigger[data-disabled] { opacity: 0.5; + cursor: not-allowed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/tabs/tabs.module.css` around lines 77 - 79, The disabled tab rule (.trigger[data-disabled]) only sets opacity, leaving the pointer cursor from .trigger intact; update the .trigger[data-disabled] selector to override the cursor (e.g., cursor: default or cursor: not-allowed) so disabled triggers no longer appear clickable and ensure the disabled selector appears after the base .trigger rule so it takes precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/raystack/components/tabs/tabs.module.css`:
- Around line 12-26: TabsRoot maps size="large" and variant="segmented" (and
other defaults) to CSS-module keys that don't exist in tabs.module.css, creating
a contract gap; add the missing modifier classes (.size-large,
.variant-segmented and any other referenced but undefined modifier classes) to
tabs.module.css and define the same CSS custom properties/values as the existing
size/variant classes (or appropriate defaults) so the class names used by
TabsRoot match actual stylesheet keys.
---
Outside diff comments:
In `@apps/www/src/content/docs/components/tabs/demo.ts`:
- Around line 6-27: The Tabs demo has mismatched values: the Tabs component sets
defaultValue="tab-one" but there is no Tabs.Tab with value "tab-one", and
several Tabs.Content entries (e.g., values "tab-one" and "tab-three") lack
corresponding Tabs.Tab triggers; update the Tabs.List to include Tabs.Tab
elements matching every Tabs.Content value or remove/rename the orphaned
Tabs.Content values to match existing Tabs.Tab triggers (ensure defaultValue
matches an existing Tabs.Tab value), referencing the Tabs component, its
defaultValue prop, Tabs.List / Tabs.Tab entries, and Tabs.Content value
attributes when making the changes.
---
Nitpick comments:
In `@packages/raystack/components/tabs/__tests__/tabs.test.tsx`:
- Around line 321-353: Add a new test in the "Sizes" suite that renders <Tabs
defaultValue='tab1' size='medium'> with a Tabs.List/Tabs.Tab and Tabs.Content
(same pattern as the existing tests), query the tablist via
screen.getByRole('tablist'), find its closest element with styles.root, assert
it's not null, and assert it has the class styles['size-medium'] to validate the
medium size option for the Tabs component.
In `@packages/raystack/components/tabs/tabs.module.css`:
- Around line 77-79: The disabled tab rule (.trigger[data-disabled]) only sets
opacity, leaving the pointer cursor from .trigger intact; update the
.trigger[data-disabled] selector to override the cursor (e.g., cursor: default
or cursor: not-allowed) so disabled triggers no longer appear clickable and
ensure the disabled selector appears after the base .trigger rule so it takes
precedence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28a946e5-eb1a-42a2-a62b-020341598fa6
📒 Files selected for processing (6)
apps/www/src/content/docs/components/tabs/demo.tsapps/www/src/content/docs/components/tabs/index.mdxapps/www/src/content/docs/components/tabs/props.tspackages/raystack/components/tabs/__tests__/tabs.test.tsxpackages/raystack/components/tabs/tabs.module.csspackages/raystack/components/tabs/tabs.tsx
Comment on lines
+12
to
26
| .size-small { | ||
| --tabs-trigger-height: var(--rs-space-6); | ||
| --tabs-trigger-padding-inline: var(--rs-space-2); | ||
| --tabs-trigger-font-size: var(--rs-font-size-mini); | ||
| --tabs-trigger-line-height: var(--rs-line-height-mini); | ||
| --tabs-trigger-letter-spacing: var(--rs-letter-spacing-mini); | ||
| } | ||
|
|
||
| .size-medium { | ||
| --tabs-trigger-height: var(--rs-space-7); | ||
| --tabs-trigger-padding-inline: var(--rs-space-3); | ||
| --tabs-trigger-font-size: var(--rs-font-size-small); | ||
| --tabs-trigger-line-height: var(--rs-line-height-small); | ||
| --tabs-trigger-letter-spacing: var(--rs-letter-spacing-small); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing default modifier classes creates a class contract gap.
TabsRoot now maps size="large" and variant="segmented" to CSS-module keys, but this stylesheet does not define .size-large or .variant-segmented. That breaks the expected root modifier contract for defaults.
💡 Suggested fix
.size-medium {
--tabs-trigger-height: var(--rs-space-7);
--tabs-trigger-padding-inline: var(--rs-space-3);
--tabs-trigger-font-size: var(--rs-font-size-small);
--tabs-trigger-line-height: var(--rs-line-height-small);
--tabs-trigger-letter-spacing: var(--rs-letter-spacing-small);
}
+
+.size-large {
+ --tabs-trigger-height: var(--rs-space-8);
+ --tabs-trigger-padding-inline: var(--rs-space-3);
+ --tabs-trigger-font-size: var(--rs-font-size-regular);
+ --tabs-trigger-line-height: var(--rs-line-height-regular);
+ --tabs-trigger-letter-spacing: var(--rs-letter-spacing-regular);
+}
@@
.variant-standalone .list,
.variant-plain .list {
background-color: transparent;
box-shadow: none;
}
+
+.variant-segmented .list {
+ background-color: var(--rs-color-background-neutral-secondary);
+ box-shadow: var(--rs-shadow-inset);
+}Also applies to: 126-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/tabs/tabs.module.css` around lines 12 - 26,
TabsRoot maps size="large" and variant="segmented" (and other defaults) to
CSS-module keys that don't exist in tabs.module.css, creating a contract gap;
add the missing modifier classes (.size-large, .variant-segmented and any other
referenced but undefined modifier classes) to tabs.module.css and define the
same CSS custom properties/values as the existing size/variant classes (or
appropriate defaults) so the class names used by TabsRoot match actual
stylesheet keys.
paanSinghCoder
changed the title
paanSinghCoder
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
feat: introduce size (small, medium and large) and variant (segmented, standalone and plain) props.
Type of Change
How Has This Been Tested?
[Describe the tests that you ran to verify your changes]
Checklist:
Screenshots (if appropriate):
[Add screenshots here]
Related Issues
[Link any related issues here using #issue-number]