Storybook update and CSS fixes #314
Merged
+304
−27
Conversation
This file contains 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
| ...props | ||
| }: ButtonProps) { | ||
| // Filter out 'icon' size as it's not supported by ShadcnButton | ||
| const buttonSize = size === 'icon' ? 'sm' : size |
There was a problem hiding this comment.
The buttonSize logic has a potential type safety issue. While it converts 'icon' to 'sm', the else case (size) could still be 'icon', which would cause a type error when passed to ShadcnButton. The function should validate all possible size values.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
|
|
||
| // export const Success2 = stories['success'] | ||
|
|
||
| // module.exports = stories |
There was a problem hiding this comment.
Using module.exports in a TypeScript/ESM file is incorrect. TypeScript with React typically uses ESM (ES Modules) where 'export' syntax should be used instead of CommonJS 'module.exports'. Additionally, this line is trying to export a variable that is commented out.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| // }), | ||
| // ) | ||
|
|
||
| // export const Success2 = stories['success'] |
There was a problem hiding this comment.
Accessing an array element using string index from a const assertion tuple type (['success', 'error'] as const) doesn't properly maintain type safety. TypeScript may not correctly infer the literal type. Consider using a type-safe mapped type or enum instead.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
|
😱 Found 3 issues. Time to roll up your sleeves! 😱 🗒️ View all ignored comments in this repo
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 3e844ff in 1 minute and 48 seconds
More details
- Looked at
479lines of code in9files - Skipped
1files when reviewing. - Skipped posting
13drafted comments based on config settings.
1. apps/web/components/ColorConfig.tsx:157
- Draft comment:
Consider memoizing the computed CSS (using useMemo) to prevent unnecessary re-renders when themeColors updates. This might improve performance slightly. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. packages/ui/global.css:39
- Draft comment:
Consistent naming observed. Just confirm that all CSS variable values match the intended design across themes. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. packages/ui/package.json:11
- Draft comment:
Using 'concurrently' is fine; consider verifying that the script names (dev:storybook, dev:tailwind) remain in sync with other parts of the repo. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. packages/ui/v1/Button.tsx:21
- Draft comment:
Mapping the 'icon' size to 'sm' is clearly documented. Consider adding a brief comment to explain that the extra padding and dimensions are manually applied for icon buttons. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. packages/ui/tailwind.config.ts:145
- Draft comment:
The custom color 'outline' with key 'hover-foreground' is defined here. Ensure naming conventions across CSS variables remain consistent with how they're referenced in components. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/web/components/ColorConfig.tsx:57
- Draft comment:
New CSS tokens (destructive and outlineHoverForeground) are added. Ensure these variables are consistently used across all themes and components. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
7. packages/ui/global.css:38
- Draft comment:
CSS variables for destructive and outline hover have been updated. Verify these tokens are in sync with component styles and theme configurations. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
8. packages/ui/package.json:11
- Draft comment:
New dev script using 'concurrently' is introduced. Confirm that the added dependency and script changes align with team practices. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
9. packages/ui/tailwind.config.ts:145
- Draft comment:
The new 'outline' color token mapping (hover-foreground) is added. Double-check that it matches the usage in component styles (e.g., Button outline variant). - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
10. apps/web/components/ColorConfig.tsx:47
- Draft comment:
Typo: The comment for buttonLight currently says 'Light Purples'. Consider changing it to 'Light Purple' for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the comment is technically correct about the typo, it's an extremely minor issue that doesn't affect functionality. Comments are documentation, not code, and this particular pluralization issue doesn't impact understanding. According to the rules, we should not make comments that are obvious or unimportant.
The typo could theoretically cause confusion if someone is searching for exact text matches in the codebase. Also, maintaining consistent documentation is important for professionalism.
The potential confusion from this pluralization issue is extremely minimal, and the professional impact is negligible. This is exactly the kind of nitpicky comment that creates noise in code reviews.
Delete this comment as it's too minor to be worth the PR author's attention. It violates the rule about not making obvious or unimportant comments.
11. packages/ui/tailwind.config.ts:6
- Draft comment:
Typo found: 'sligthly' should be corrected to 'slightly' on line 6. Please fix for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. packages/ui/v1/Button.stories.ts:99
- Draft comment:
There is a missing newline at end of file. Please add a newline after line 99 for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While having a newline at the end of files is a common convention, this is a very minor formatting issue that would typically be handled by EditorConfig, Prettier, or other automated formatting tools. It doesn't affect functionality and is too trivial to warrant a manual review comment. This falls under the "Do NOT make comments that are obvious or unimportant" rule.
Missing newlines at end of file can cause issues with some tools and git diffs. Some would argue this is a valid formatting standard to enforce.
While true, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than manual review comments. If it's important to the project, it should be enforced via tooling.
This comment should be deleted as it's too minor of an issue to warrant a manual review comment and would be better handled by automated formatting tools.
13. packages/ui/v1/Button.tsx:34
- Draft comment:
The file is missing a newline at the end. Please add a newline at the end of the file for consistency and to adhere to common style guidelines. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While missing trailing newlines can cause issues with some tools and is generally considered good practice, this is a very minor style issue that would likely be caught by linting or formatting tools. It's not a logic issue and doesn't affect functionality. According to our rules, we shouldn't comment on purely stylistic issues.
The missing newline could potentially cause merge conflicts or diff issues in some cases. Some would argue this is more than just style.
While true, this is still the kind of issue that should be handled by automated tooling rather than manual review comments. Most modern development environments handle this automatically.
This comment should be deleted as it addresses a minor style issue that should be handled by automated tooling rather than manual review.
Workflow ID: wflow_RAfhm4eeLlpQviBh
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| args: {status: 'error'}, | ||
| } | ||
|
|
||
| // Not working yet |
There was a problem hiding this comment.
Consider removing or cleaning up commented-out story code if it's not intended for future use to reduce clutter.
Suggested change
| // Not working yet |
| const buttonSize = size === 'icon' ? 'sm' : size | ||
|
|
||
| return ( | ||
| <ShadcnButton |
There was a problem hiding this comment.
The 'asChild' prop is destructured but never forwarded to ShadcnButton. If intended for polymorphic rendering, pass it down or remove it.
snrondina
added a commit
that referenced
this pull request
Mar 11, 2025
* Add StatusBadge example * dev command * storybook update and css fixes --------- Co-authored-by: OpenInt Bot <[email protected]>
openint-bot
added a commit
that referenced
this pull request
Mar 11, 2025
* Add StatusBadge example * dev command * storybook update and css fixes --------- Co-authored-by: OpenInt Bot <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Important
Update Storybook setup, add new components, and enhance CSS styles for better UI consistency.
Button.stories.tsandStatusBadge.stories.tsforButtonandStatusBadgecomponents.package.jsonscripts to includedev:storybookanddev:tailwindfor concurrent development.Button.tsxandStatusBadge.tsxinv1directory with variants and size options.Buttoncomponent inshadcn/Button.tsxto include new styles fordestructive,outline, andsecondaryvariants.global.cssandColorConfig.tsxto include new color variables fordestructiveandoutlineHoverForeground.tailwind.config.tsto extend theme with new color configurations.This description was created by
for 3e844ff. It will automatically update as commits are pushed.