SearchBar & OverflowStack #981
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces two new UI components (SearchBar and OverflowStack) with comprehensive documentation, updates the SearchBar component styling to use CSS variables, and fixes a text utility variable name.
- Adds SearchBar component with search-on-type, external triggering, and debouncing capabilities
- Adds OverflowStack component for managing item overflow with popover support
- Updates SearchBar CSS to use modern CSS custom properties instead of SCSS variables
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/pages/components/utilities/TextUtilities.tsx | Corrects CSS variable name from --text-size-x-small to --text-size-xx-small |
| examples/pages/components/SearchBar.tsx | Adds comprehensive documentation page for SearchBar component with usage examples |
| examples/pages/components/OverflowStack.tsx | Adds comprehensive documentation page for OverflowStack component with usage examples |
| examples/pages/components/Index.tsx | Registers the new SearchBar and OverflowStack documentation pages |
| app/styles/components/_sd-searchbar.scss | Migrates from SCSS variables to CSS custom properties and removes empty lines |
| app/styles/components/_overflow-stack.scss | Adds new stylesheet for OverflowStack component |
| app/styles/app.scss | Imports the new overflow-stack stylesheet |
| app/styles/_reboot.scss | Minor formatting adjustments for comments |
| app-typescript/index.ts | Exports the new OverflowStack component |
| app-typescript/components/SearchBar.tsx | Enhances SearchBar with refs, debouncing, external methods, and searchOnType feature |
| app-typescript/components/OverflowStack.tsx | Adds new OverflowStack component implementation with auto and fixed overflow modes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| componentDidUpdate(prevProps: IProps) { | ||
| if (prevProps.value !== this.props.value) { | ||
| this.setState({inputValue: this.props.value}); | ||
| this.setState({inputValue: this.props.value || ''}); |
There was a problem hiding this comment.
Component state update uses potentially inconsistent value.
| contentTypes: ContentTypeItem[]; | ||
| } | ||
|
|
||
| export default class OverflowStackDoc extends React.Component<IProps, IState> { |
There was a problem hiding this comment.
Component state property 'value1' is written, but it is never read.
Component state property 'value1' is written, but it is never read.
Component state property 'value2' is written, but it is never read.
Component state property 'value2' is written, but it is never read.
Component state property 'value3' is written, but it is never read.
Component state property 'value3' is written, but it is never read.
Component state property 'value4' is written, but it is never read.
Component state property 'value4' is written, but it is never read.
Component state property 'value5' is written, but it is never read.
Component state property 'value5' is written, but it is never read.
Component state property 'value6' is written, but it is never read.
Component state property 'value6' is written, but it is never read.
| import { | ||
| PropsList, | ||
| Prop, | ||
| OverflowStack, | ||
| Badge, | ||
| Tag, | ||
| Button, | ||
| ButtonGroup, | ||
| IconButton, | ||
| Label, | ||
| CheckboxButton, | ||
| ResizablePanels, | ||
| Checkbox, | ||
| } from '../../../app-typescript'; |
There was a problem hiding this comment.
Unused imports Badge, Button, ButtonGroup.
…rdesk-ui-framework into SearchBox-and-OverflowStack
|
hi @fritzSF could you in the future make smaller PRs that are focused on one component/feature area? It's a bit hard to review a bunch of things with a lot of line changes. Reviews will be of better quality if we do that I think Edit: if the feature itself is single and too big, we can split it in parts. I didn't cover that case with my comment |
| hideSearchButton?: boolean; // Hide the internal search button (useful when triggering search externally) | ||
| searchOnType?: boolean; // Enable automatic search while typing (debounced) | ||
| searchDelay?: number; // Delay in milliseconds for searchOnType (default: 300) |
There was a problem hiding this comment.
we can use discriminated union here to make the interface cleaner and the component easier to use I think. Maybe we could even separate the core of the component from these two types of search and make two. Let me know if you can implement it with Cursor, I can also implement it
| inputValue: this.props.value || '', | ||
| focused: this.props.focused || false, | ||
| type: this.props.type || 'expanded', | ||
| boxed: this.props.boxed || false, |
There was a problem hiding this comment.
we can keep the existing more explicit code style here. Also the difference between || and ?? is that || will take any falsy value - i.e. "" and undefined counts as falsy in the case of ||, which might not be the intended behaviour. same goes for expressions using ? :
2 participants
Changes on the SerchBar component
New OverflowStack component
A flexible container that stacks any child elements. It can display a fixed number of items and move the remainder into a popover. Optionally, it can automatically adjust which items are visible based on available space, hiding the excess in the popover.
Features: