chore: Button Refactor - BED-6463 - #3030
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces the legacy button implementation with Base UI-backed ChangesShared button migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
…se, remove bad focus styling, update Button again
KillahDillah
marked this pull request as ready for review
July 30, 2026 21:43
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/javascript/doodle-ui/src/tailwind/plugin.ts (1)
519-526: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCommenting out
--tw-ring-colorleaves--tw-ring-shadowresolving to Tailwind's default ring color.
--tw-ring-shadowon line 525 still interpolatesvar(--tw-ring-color), which Tailwind's base layer defaults to its blue (rgb(59 130 246 / 0.5)). So.focus-ringnow paints a correct-coloredoutlineplus a blue box-shadow ring underneath it. If the box-shadow ring is no longer wanted, remove the ring variables/box-shadow entirely rather than only the color.🎨 Option: keep only the outline-based ring
'.focus-ring': { outline: 'var(--focus-ring-width) solid var(--focus-ring)', 'outline-offset': 'var(--focus-ring-offset-width)', - '--tw-ring-offset-width': 'var(--focus-ring-offset-width)', - '--tw-ring-offset-color': 'var(--focus-ring-offset)', - // TODO causes issues with Text - // '--tw-ring-color': 'var(--focus-ring)', - '--tw-ring-offset-shadow': '0 0 0 var(--tw-ring-offset-width) var(--tw-ring-offset-color)', - '--tw-ring-shadow': - '0 0 0 calc(var(--focus-ring-width) + var(--tw-ring-offset-width)) var(--tw-ring-color)', - 'box-shadow': 'var(--tw-ring-offset-shadow), var(--tw-ring-shadow), var(--tw-shadow, 0 0 `#0000`)', },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/doodle-ui/src/tailwind/plugin.ts` around lines 519 - 526, Update the focus-ring styles around the commented-out --tw-ring-color and --tw-ring-shadow declarations so the component no longer renders Tailwind’s default blue box-shadow ring; remove the ring-related variables and box-shadow when keeping the outline-only behavior, while preserving the correctly colored outline.
🧹 Nitpick comments (8)
packages/javascript/doodle-ui/src/components/Button/Button.stories.tsx (2)
135-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the commented-out
variantargType block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/doodle-ui/src/components/Button/Button.stories.tsx` around lines 135 - 146, Remove the commented-out variant entry from the argTypes configuration, leaving the active fontColor configuration unchanged.
204-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
aria-labelvalues don't match the rendered icons.Line 210 labels a star icon as "Gear Icon", and line 216 labels a filter icon the same way. Since these stories are the reference for
IconButtonaccessibility usage, the labels should describe the actual action/icon.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/doodle-ui/src/components/Button/Button.stories.tsx` around lines 204 - 218, The aria-label values on the secondary and disabled IconButtonComponent examples do not match their rendered icons. Update the labels in the Button stories so the secondary star button describes the star icon and the disabled AppIcon.FilterOutline button describes the filter icon, while leaving the primary example unchanged.packages/javascript/doodle-ui/src/components/Calendar/Calendar.tsx (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the package entrypoint over the deep path.
TextButtonVariantsis re-exported from../Button; importing from../Button/Buttonbypasses the module boundary the index file establishes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/doodle-ui/src/components/Calendar/Calendar.tsx` at line 20, Update the TextButtonVariants import in Calendar.tsx to use the ../Button package entrypoint instead of the deep ../Button/Button path, preserving the existing symbol usage.packages/javascript/doodle-ui/src/components/Button/index.ts (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso export the new prop types.
TextButtonProps/IconButtonProps(andIconButtonVariants) aren't re-exported, so wrapper components inbh-shared-uimust deep-import from./Buttonto type their props.♻️ Proposed export
-export { Button, ButtonVariants, IconButton, TextButton, TextButtonVariants, type ButtonProps } from './Button'; +export { + Button, + ButtonVariants, + IconButton, + IconButtonVariants, + TextButton, + TextButtonVariants, + type ButtonProps, + type IconButtonProps, + type TextButtonProps, +} from './Button';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/doodle-ui/src/components/Button/index.ts` at line 16, Update the barrel export in the Button component index to re-export TextButtonProps, IconButtonProps, and IconButtonVariants alongside the existing Button symbols, allowing consumers to import these prop types without deep-importing the implementation module.packages/javascript/doodle-ui/src/components/Button/Button.tsx (2)
69-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
fontColoronButtonVariantsis now dead surface.Its own TODO says the only consumer was
variant="text", which this PR removed, andTextButtonownsfontColorgoing forward. Consider dropping it fromButtonVariants/ButtonPropsso the publicButtonAPI doesn't expose a no-op prop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/doodle-ui/src/components/Button/Button.tsx` around lines 69 - 72, Remove the obsolete fontColor variant from ButtonVariants and ButtonProps, including its primary text-primary mapping, since Button no longer consumes it. Keep fontColor owned by TextButton and ensure the public Button API no longer exposes this no-op prop.
202-206: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated sizing/shape classes.
IconButtonVariantsalready emitsrounded-full border-0 p-2andsize-fit/shrink-0; repeating them inline (plushas-[svg]:p-2) just adds twMerge work and two places to keep in sync. Fold the extra classes into thecvabase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/doodle-ui/src/components/Button/Button.tsx` around lines 202 - 206, Update the cva base used by IconButtonVariants to include the shared sizing and shape classes currently duplicated in the Button className, including the SVG padding behavior; then remove those duplicates from the inline class list while preserving unrelated classes and variant handling.packages/javascript/doodle-ui/src/tailwind/plugin.ts (1)
129-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDangling
// TODO remove.The marker is only added in the light
:rootblock; the same--tertiary-btn-borderin.dark(line 373) is unannotated, so the cleanup will be easy to miss. Want me to open a tracking issue?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/doodle-ui/src/tailwind/plugin.ts` around lines 129 - 130, Remove the dangling TODO comment above --tertiary-btn-border in the light :root block; leave the variable and the corresponding dark-mode definition unchanged.packages/javascript/bh-shared-ui/src/components/LabelWithCopy.tsx (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
IconButtonfor this icon-only control.This renders only a copy icon;
packages/javascript/bh-shared-ui/src/components/Icon.tsxusesIconButtonfor the samearia-label+ icon pattern. Using it here keeps icon-only sizing and focus behavior consistent.Proposed change
-import { TextButton, Tooltip } from 'doodle-ui'; +import { IconButton, Tooltip } from 'doodle-ui'; - <TextButton + <IconButton onClick={handleCopy} aria-label='Copy to clipboard' className={cn('text-inherit text-sm', { invisible: !(!hoverOnly || hoverActive) })}> <FontAwesomeIcon icon={faCopy} /> - </TextButton> + </IconButton>Also applies to: 52-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/bh-shared-ui/src/components/LabelWithCopy.tsx` at line 19, Update the icon-only copy control in LabelWithCopy to use IconButton instead of TextButton, following the aria-label and icon usage established in Icon.tsx. Preserve the existing copy action and accessible label while adopting IconButton’s consistent sizing and focus behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 57-59: Move the `@base-ui/react` dependency from the root
package.json dependencies into packages/javascript/doodle-ui/package.json,
preserving its current version range. Ensure the doodle-ui package directly
declares the runtime dependency required by its `@base-ui/react/button` import.
In
`@packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestFilterDialog.tsx`:
- Around line 97-102: Update the IconButton in
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestFilterDialog.tsx
lines 97-102 to use the accessible name “Open file ingest filters”. In
packages/javascript/bh-shared-ui/src/components/FileIngest/FileIngest.test.tsx
line 98 and
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx
line 243, replace data-testid queries with role-based button queries matching
that accessible name.
In
`@packages/javascript/bh-shared-ui/src/components/HelpTexts/CodeController/CodeController.tsx`:
- Line 128: Replace the div wrapper in the CodeController markup with a phrasing
element, preserving its existing className and contents, so it remains valid
inside the Typography component='pre' container.
In
`@packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx`:
- Around line 339-340: Update the class names in the CypherSearch editor styling
to use descendant selectors for nested CodeMirror elements: change the
cm-content and cm-tooltip selectors in the affected class list to the underscore
descendant forms, preserving all other classes and behavior.
In
`@packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/CommonSearches.tsx`:
- Around line 182-188: Update the TextButton rendering in CommonSearches to
expose the saved-queries state with aria-expanded bound to showCommonQueries and
aria-controls referencing the controlled panel’s existing id. Ensure the panel
has a matching stable id while preserving the current toggle behavior.
In
`@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Filters/DateField.tsx`:
- Around line 60-68: Add an accessible aria-label to the icon-only TextButton in
the date field clear-control handler, identifying that it clears the selected
date. Preserve the existing disabled state, visibility styling, and
form.setValue/form.clearErrors behavior.
In
`@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.tsx`:
- Around line 126-130: Update the confirm TextButton in FilterDialog so it no
longer triggers native form submission after handleConfirm; use a non-submit
button type while preserving the existing click handler and confirmation
behavior.
In
`@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx`:
- Around line 158-162: Update the IconButton in GlyphSelectDialog to use an
accessible name that describes clearing the selection, replacing the
glyph-focused aria-label; remove or correct the invalid aria-describedby value
so it references an actual element ID only if needed.
In `@packages/javascript/doodle-ui/src/components/Button/Button.stories.tsx`:
- Around line 36-42: Update the Button story controls to expose only currently
supported values: remove `text` from the `options` array and remove `default`
from `fontColor.options`, leaving `primary` as the sole font color option. Clean
up the associated stale TODO/commented configuration in the story.
In `@packages/javascript/doodle-ui/src/components/Button/Button.tsx`:
- Around line 21-28: The shared buttonBaseClasses currently applies
hover:underline to filled variants; remove that utility from the shared base and
add it to TextButtonBaseClasses so only text-style buttons underline on hover.
Preserve transparent, primary, secondary, and IconButton hover behavior without
introducing per-variant resets.
In `@packages/javascript/doodle-ui/src/components/Calendar/Calendar.tsx`:
- Line 58: Invoke the TextButtonVariants cva factory before passing its result
to cn/clsx in the Calendar configuration, updating nav_button, day, and the
caption SelectTrigger className. Preserve the existing text-button variant
styling by using the factory with the text variant rather than passing
TextButtonVariants itself.
In `@packages/javascript/doodle-ui/src/components/Carousel/Carousel.tsx`:
- Around line 211-220: Update CarouselPrev and CarouselNext in
packages/javascript/doodle-ui/src/components/Carousel/Carousel.tsx at lines
211-220 and 257-266: type both forwardRef props as React.ComponentProps<typeof
TextButton> instead of Button props, and remove size='small' from both
TextButton usages.
---
Outside diff comments:
In `@packages/javascript/doodle-ui/src/tailwind/plugin.ts`:
- Around line 519-526: Update the focus-ring styles around the commented-out
--tw-ring-color and --tw-ring-shadow declarations so the component no longer
renders Tailwind’s default blue box-shadow ring; remove the ring-related
variables and box-shadow when keeping the outline-only behavior, while
preserving the correctly colored outline.
---
Nitpick comments:
In `@packages/javascript/bh-shared-ui/src/components/LabelWithCopy.tsx`:
- Line 19: Update the icon-only copy control in LabelWithCopy to use IconButton
instead of TextButton, following the aria-label and icon usage established in
Icon.tsx. Preserve the existing copy action and accessible label while adopting
IconButton’s consistent sizing and focus behavior.
In `@packages/javascript/doodle-ui/src/components/Button/Button.stories.tsx`:
- Around line 135-146: Remove the commented-out variant entry from the argTypes
configuration, leaving the active fontColor configuration unchanged.
- Around line 204-218: The aria-label values on the secondary and disabled
IconButtonComponent examples do not match their rendered icons. Update the
labels in the Button stories so the secondary star button describes the star
icon and the disabled AppIcon.FilterOutline button describes the filter icon,
while leaving the primary example unchanged.
In `@packages/javascript/doodle-ui/src/components/Button/Button.tsx`:
- Around line 69-72: Remove the obsolete fontColor variant from ButtonVariants
and ButtonProps, including its primary text-primary mapping, since Button no
longer consumes it. Keep fontColor owned by TextButton and ensure the public
Button API no longer exposes this no-op prop.
- Around line 202-206: Update the cva base used by IconButtonVariants to include
the shared sizing and shape classes currently duplicated in the Button
className, including the SVG padding behavior; then remove those duplicates from
the inline class list while preserving unrelated classes and variant handling.
In `@packages/javascript/doodle-ui/src/components/Button/index.ts`:
- Line 16: Update the barrel export in the Button component index to re-export
TextButtonProps, IconButtonProps, and IconButtonVariants alongside the existing
Button symbols, allowing consumers to import these prop types without
deep-importing the implementation module.
In `@packages/javascript/doodle-ui/src/components/Calendar/Calendar.tsx`:
- Line 20: Update the TextButtonVariants import in Calendar.tsx to use the
../Button package entrypoint instead of the deep ../Button/Button path,
preserving the existing symbol usage.
In `@packages/javascript/doodle-ui/src/tailwind/plugin.ts`:
- Around line 129-130: Remove the dangling TODO comment above
--tertiary-btn-border in the light :root block; leave the variable and the
corresponding dark-mode definition unchanged.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7d5dcd0d-4779-4f0b-9dda-ddb07c5c24af
⛔ Files ignored due to path filters (10)
.yarn/cache/@babel-runtime-npm-7.29.7-423e8cd4b6-9883b49517.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@base-ui-react-npm-1.6.0-b96a743958-7fed3b731e.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@base-ui-utils-npm-0.3.1-7fda044f8e-48fe7f0b1a.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@floating-ui-core-npm-1.8.0-7cb6ad2d03-762cd5677f.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@floating-ui-dom-npm-1.8.0-a6d2aa48a8-2e7d75fb70.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@floating-ui-react-dom-npm-2.1.9-751127e598-d775609760.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@floating-ui-utils-npm-0.2.12-72e5a495e8-ec4e176fb2.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/reselect-npm-5.2.0-4fd7475b1f-e53d37a35f.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/use-sync-external-store-npm-1.6.0-2db2af616d-b40ad2847b.zipis excluded by!**/.yarn/**,!**/*.zipyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (60)
cmd/ui/src/views/EarlyAccessFeatures/EarlyAccessFeatures.tsxpackage.jsonpackages/javascript/bh-shared-ui/src/components/AnalyzeNowConfiguration/AnalyzeNowConfirmDialog.tsxpackages/javascript/bh-shared-ui/src/components/CitrixRDPConfiguration/CitrixRDPConfirmDialog.tsxpackages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsxpackages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownSelector.tsxpackages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTriggerContents.tsxpackages/javascript/bh-shared-ui/src/components/DropdownSelector/constants.tspackages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.tsxpackages/javascript/bh-shared-ui/src/components/ExploreTable/TableControls.tsxpackages/javascript/bh-shared-ui/src/components/FileIngest/FileIngest.test.tsxpackages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestFilterDialog.tsxpackages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/CodeController/CodeController.tsxpackages/javascript/bh-shared-ui/src/components/Icon.tsxpackages/javascript/bh-shared-ui/src/components/LabelWithCopy.tsxpackages/javascript/bh-shared-ui/src/components/LoginForm/LoginForm.tsxpackages/javascript/bh-shared-ui/src/components/LoginViaSSOForm/LoginViaSSOForm.tsxpackages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsxpackages/javascript/bh-shared-ui/src/components/SSOProviderTable/SSOProviderTable.tsxpackages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsxpackages/javascript/bh-shared-ui/src/components/UserTokenManagementDialog/UserTokenManagementDialog.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter/EdgeFilteringDialog.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/CommonSearches.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmDeleteQueryDialog.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ImportQueryDialog.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/QuerySearchFilter.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryDialog.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabel.tsxpackages/javascript/bh-shared-ui/src/views/OpenGraphManagement/ActiveExtensionsCard.tsxpackages/javascript/bh-shared-ui/src/views/OpenGraphManagement/DeleteExtensionButton.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/ObjectsAccordion.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesAccordion.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Filters/DateField.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Filters/TagIdField.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/ActionField.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/MadeByField.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/NoteCell.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/DeleteRuleButton/DeleteRuleButton.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/ObjectSelect.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/SeedSelection.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/SeedSelectionPreview.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/fragments.tsxpackages/javascript/doodle-ui/src/components/Button/Button.stories.tsxpackages/javascript/doodle-ui/src/components/Button/Button.tsxpackages/javascript/doodle-ui/src/components/Button/index.tspackages/javascript/doodle-ui/src/components/Calendar/Calendar.tsxpackages/javascript/doodle-ui/src/components/Card/Card.tsxpackages/javascript/doodle-ui/src/components/Carousel/Carousel.tsxpackages/javascript/doodle-ui/src/components/DatePicker/DatePicker.tsxpackages/javascript/doodle-ui/src/tailwind/plugin.tspackages/javascript/doodle-ui/src/tailwind/preset.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTriggerContents.tsx (1)
50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the render-time debug log.
console.log(open)runs on every render and will pollute production browser consoles.Proposed fix
- console.log(open);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTriggerContents.tsx` at line 50, Remove the console.log(open) debug statement from the DropdownTriggerContents render path, leaving the surrounding component behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTriggerContents.tsx`:
- Line 50: Remove the console.log(open) debug statement from the
DropdownTriggerContents render path, leaving the surrounding component behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5c1b5cb5-7661-4524-a6fa-44dcc362c602
📒 Files selected for processing (2)
packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTriggerContents.tsxpackages/javascript/bh-shared-ui/src/components/DropdownSelector/constants.ts
KillahDillah
force-pushed
the
BED-6463-button-refactor
branch
from
July 30, 2026 22:41
93207eb to
7d31fd0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/javascript/doodle-ui/src/components/Carousel/Carousel.tsx`:
- Around line 212-220: Add accessible labels to both carousel controls: update
the previous-slide TextButton near Carousel controls at
packages/javascript/doodle-ui/src/components/Carousel/Carousel.tsx lines 212-220
with aria-label “Previous slide”, and the next-slide TextButton at lines 259-267
with aria-label “Next slide”.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 06794829-585c-46f8-a3af-f2326f782995
📒 Files selected for processing (9)
packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTriggerContents.tsxpackages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestFilterDialog.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/CodeController/CodeController.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/CommonSearches.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsxpackages/javascript/doodle-ui/src/components/Button/Button.stories.tsxpackages/javascript/doodle-ui/src/components/Calendar/Calendar.tsxpackages/javascript/doodle-ui/src/components/Carousel/Carousel.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/CommonSearches.tsx
- packages/javascript/doodle-ui/src/components/Calendar/Calendar.tsx
- packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestFilterDialog.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx
Holocraft
reviewed
Aug 3, 2026
|
|
||
| <DialogActions> | ||
| <Button variant={'text'} className='p-2' onClick={closeDialog}> | ||
| <TextButton className='pr-2' onClick={closeDialog}> |
Contributor
There was a problem hiding this comment.
nit: can remove pr-2 as it gets padding from the component.
Holocraft
reviewed
Aug 3, 2026
| className={cn('text-contrast', iconClasses)} | ||
| data-testid='tier_zero_icon' | ||
| /> | ||
| <AppIcon.TierZero {...iconProps} size={16} className={cn(iconClasses)} data-testid='tier_zero_icon' /> |
Contributor
There was a problem hiding this comment.
No need for the cn utility here.
specter-flq
reviewed
Aug 6, 2026
specter-flq
reviewed
Aug 6, 2026
specter-flq
reviewed
Aug 6, 2026
| it('displays a Filters button', async () => { | ||
| await act(async () => render(<FileIngest />)); | ||
| const filterButton = screen.getByRole('button', { name: /app-icon-filter-outline/i }); | ||
| const filterButton = screen.getByTestId('file_ingest_log-open_filter_dialog'); |
Contributor
There was a problem hiding this comment.
We should consider getting by role over testId I think thats the preferred priority of the library. Also the rabbit makes a comment about it below.
specter-flq
reviewed
Aug 6, 2026
specter-flq
reviewed
Aug 6, 2026
specter-flq
reviewed
Aug 6, 2026
specter-flq
reviewed
Aug 6, 2026
This was referenced Aug 10, 2026
TheNando
reviewed
Aug 10, 2026
TheNando
left a comment
Contributor
There was a problem hiding this comment.
Heroic effort! Just got a handful of comments.
| aria-label='Toggle search widget' | ||
| data-testid='explore_search-container_header_expand-collapse-button' | ||
| className={classes.icon} | ||
| className='ml-1 px-4 rounded-none text-sm' |
Contributor
There was a problem hiding this comment.
Two conventions that apply here and should commonly be considered across the codebase:
- Whenever possible, isolate layout clases to container components (the div with the icon button and the tabs)
- Always question if CSS style overrides/additions represent a reusable case/variant (in which case, the style should be applied in Doodle as a variant) or an exceptional case where they must exist at the feature component level. Exceptional cases should be less common than variants.
With that in mind, I think you can entirely remove IconButton classes here. Left margin can become padding in the parent. The right padding can be collapsed into an increased gap for the parent. rounded-none and text-sm don't seem to do anything for IconButton.
I won't bog down this PR with every mention of this type of change, but I thought this was a good example to illustrate.
Contributor
Author
There was a problem hiding this comment.
So, rounded-none comes into place for the focus styles. Instead of a focus ring, it's a square to better match the Tabs next to it, kinda. But it looks a lot better without a border radius than with one, that's for sure.
text-sm makes the icon smaller, which matches the previous designs. I'm uncertain about making a small IconButton variant as our new designs do not reflect this.
This component but on the BHE side is using the Icon component, which I am leaving a note to replace in an upcoming ticket.
Contributor
Author
There was a problem hiding this comment.
checking with designer on rounded or non-rounded focus styles.
TheNando
approved these changes
Aug 11, 2026
KillahDillah
force-pushed
the
BED-6463-button-refactor
branch
4 times, most recently
from
August 12, 2026 16:25
6cb0787 to
c0be508
Compare
…n columnHeaders
KillahDillah
force-pushed
the
BED-6463-button-refactor
branch
from
August 12, 2026 16:31
c0be508 to
f164695
Compare
…ge focus styles
KillahDillah
force-pushed
the
BED-6463-button-refactor
branch
from
August 12, 2026 18:30
a503bcf to
b6ae722
Compare
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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
Button refactor!!!
Updated Button base with BaseUI, updated tests, replaces Icon and Text Buttons with new TextButton and IconButton. Updated color scheme. Left tons of notes for future tickets.
Motivation and Context
Resolves BED-6463
Figma links:
https://www.figma.com/design/pqlfSCxXVB5MjX5Gq50rW8/Alpha-%F0%9F%90%95?node-id=9-12&p=f&m=dev
https://www.figma.com/design/pqlfSCxXVB5MjX5Gq50rW8/Alpha-%F0%9F%90%95?node-id=431-1937&m=dev
Why is this change required? What problem does it solve?
Updated styling for Button,
split text variant and icon variant
replaced old text and icon buttons to use the new version
removed bad focus styling
and other no broken glass areas
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Manually with screen reader and keyboard navigation, visually, test suite passes
Screenshots (optional):
Updated styling:
OLD:
NEW:
OLD:
NEW:
OLD:
NEW:
OLD:
NEW:
OLD:
NEW:
OLD:
NEW:
OLD:
NEW:
STORYBOOK:
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests