-
Notifications
You must be signed in to change notification settings - Fork 71
[LG-5098] feat(CodeEditor): adds custom search panel #3186
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 8f3cab1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Change: +5.13 kB (+0.32%) Total Size: 1.6 MB
ℹ️ View Unchanged
|
…ality and styling - Added the SearchForm component to the CodeEditor, featuring a toggle button for expanding and collapsing the search input. - Integrated LeafyGreen UI components for consistent styling and functionality. - Created a new story for SearchForm in Storybook to demonstrate its usage and appearance. - Updated CodeEditor to include the SearchForm, enhancing user interaction capabilities.
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.
Pull Request Overview
This PR adds a custom search panel to the CodeEditor component that replaces CodeMirror's default search UI with one that matches LeafyGreen's design language. The new search panel includes all the same functionality as the built-in panel (find, replace, case sensitivity, regex, etc.) but with improved styling and UX consistency.
- Introduces a custom SearchPanel component with React-based UI controls
- Adds an
enableSearchPanel
prop to allow consumers to control search functionality - Conditionally loads the CodeMirror search module only when search is enabled
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
packages/code-editor/src/SearchPanel/ | New SearchPanel component with React UI and styles |
packages/code-editor/src/CodeEditor/CodeEditor.tsx | Integrates custom search panel with conditional loading |
packages/code-editor/src/CodeEditor/hooks/useModuleLoaders.ts | Makes search module loading conditional based on enableSearchPanel prop |
packages/code-editor/src/CodeEditor/CodeEditor.types.ts | Adds enableSearchPanel prop and updates CSS selectors |
packages/code-editor/src/CodeEditor/hooks/extensions/useThemeExtension.ts | Updates styling to accommodate the new search panel |
packages/code-editor/src/CodeEditor/CodeEditor.spec.tsx | Comprehensive test coverage for search panel functionality |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
<div | ||
className={replaceSectionStyles} | ||
// @ts-expect-error - react type issue: https://github.com/facebook/react/pull/24730 | ||
inert={!isOpen ? 'inert' : undefined} |
Copilot
AI
Oct 7, 2025
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.
The inert
attribute should be a boolean, not a string. Use inert={!isOpen}
instead of inert={!isOpen ? 'inert' : undefined}
.
inert={!isOpen ? 'inert' : undefined} | |
inert={!isOpen} |
Copilot uses AI. Check for mistakes.
const { createRoot } = require('react-dom/client'); | ||
const root = createRoot(dom); | ||
root.render(searchPanelElement); | ||
|
||
return { | ||
dom, | ||
top: true, | ||
unmount: () => root.unmount(), |
Copilot
AI
Oct 7, 2025
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.
Using require()
inside a function can impact tree-shaking and bundling optimization. Consider using dynamic imports with import()
or moving this to a top-level conditional import.
const { createRoot } = require('react-dom/client'); | |
const root = createRoot(dom); | |
root.render(searchPanelElement); | |
return { | |
dom, | |
top: true, | |
unmount: () => root.unmount(), | |
// Use a placeholder while loading react-dom/client dynamically | |
let root: any = null; | |
(async () => { | |
const { createRoot } = await import('react-dom/client'); | |
root = createRoot(dom); | |
root.render(searchPanelElement); | |
})(); | |
return { | |
dom, | |
top: true, | |
unmount: () => { | |
if (root) { | |
root.unmount(); | |
} | |
}, |
Copilot uses AI. Check for mistakes.
packages/code-editor/README.md
Outdated
| `enableCodeFolding` _(optional)_ | Enables code folding arrows in the gutter. | `boolean` | `undefined` | | ||
| `enableLineNumbers` _(optional)_ | Enables line numbers in the editor’s gutter. | `boolean` | `true` | | ||
| `enableLineWrapping` _(optional)_ | Enables line wrapping when the text exceeds the editor’s width. | `boolean` | `true` | | ||
| `enableSearchPanel` \_(optional)) | Enables the find and replace search panel in the editor. | `boolean` | `true` | |
Copilot
AI
Oct 7, 2025
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.
Corrected extra parenthesis and underscore formatting in the table row.
| `enableSearchPanel` \_(optional)) | Enables the find and replace search panel in the editor. | `boolean` | `true` | | |
| `enableSearchPanel` _(optional)_ | Enables the find and replace search panel in the editor. | `boolean` | `true` | |
Copilot uses AI. Check for mistakes.
packages/code-editor/README.md
Outdated
| `enableCodeFolding` _(optional)_ | Enables code folding arrows in the gutter. | `boolean` | `undefined` | | ||
| `enableLineNumbers` _(optional)_ | Enables line numbers in the editor’s gutter. | `boolean` | `true` | | ||
| `enableLineWrapping` _(optional)_ | Enables line wrapping when the text exceeds the editor’s width. | `boolean` | `true` | | ||
| `enableSearchPanel` _(optional)_ | Enables the find and replace search panel in the editor. | `boolean` | `true` | |
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.
What do you think about including a note here or elsewhere in the README about how to open/close this?
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.
Can't hurt, will do!
const [isCaseSensitive, setIsCaseSensitive] = useState(false); | ||
const [isWholeWord, setIsWholeWord] = useState(false); | ||
const [isRegex, setIsRegex] = useState(false); | ||
const [query, setQuery] = useState<SearchQuery>( |
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.
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.
Yeah, I would agree. We can make them whatever we want, these are just the defaults in CodeMirrors. @sandynguyenn this may be more a question for you - is there another color you'd prefer us to set these yellow highlights to?
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.
Spoke with Sandy who didn't feel strongly, but recommended aligning with the Code
highlight color: https://www.figma.com/design/4h2VwjCuJUbeZ7hzD2J1rq/LeafyGreen-Design-System?m=auto&node-id=29198-325819&t=S3xLTLPhXkq6XQ3w-1
I've pushed up commit 6bf2b88 to address this
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.
Oops, need to update dark mode
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.
Reverting these changes until I sync with Sandy today
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.
Sandy and I went with 2 palette colors and made the text the light theme base font color for the highlights. This should be updated now
}), | ||
); | ||
const [findCount, setFindCount] = useState(0); | ||
const { theme } = useDarkMode(darkMode); |
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.
It looks like dark mode implementation may need another look
hovering IconButton
instance turns svg black
focusing IconButton
instance turns svg black
disabled/enabled IconButton
instances look reversed; I'm not sure if that's because the background, but can we check that out? Also can compare how the format and copy buttons appear vs the ones in the SearchPanel
const findOptions = { | ||
isCaseSensitive: 'Match case', | ||
isRegex: 'Regexp', | ||
isWholeWord: 'By word', | ||
} as const; |
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.
nit: could define these outside the component
commands.history(), | ||
searchModule.search(), | ||
|
||
enableSearchPanel && searchModule |
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.
would it improve readability/maintainability to break up these code editor chunks?
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.
Break up the whole effect? I broke this effect up a bit in the QA PR that's not merged yet. Not quite as much as we could, but I did separate the initialization from the configuration https://github.com/mongodb/leafygreen-ui/compare/LG-5446/ce-qa?expand=1#diff-85df5eb3676e4f9ebec8364e075627e7b4aa40362faa78ab3294256d336cfd91R250
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.
I think I was thinking along the lines of breaking out the hooks into separate pieces with explicit, readable names to quickly identify what each one is doing. This way, CodeEditor
file is more easily parseable. Just an idea though; you can decide if that's helpful or overkill
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.
I'm not opposed to this, I'd rather just consider that in the QA branch since I'm already refactoring this effect there, might be easier on the conflicts 🙂
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.
I've actually backtracked on this. For the search panel logic, I'm creating a hook that just encapsulates this logic. Since it returns an extension, it fits nicely into the use*Extension hooks, and will actually make the conflicts easier to deal with because they'll be less changes in the CodeEditor
component itself. Pushing shortly
[css` | ||
border-top-right-radius: ${borderRadius[300]}px; | ||
`]: !hasPanel, |
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.
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.
Oof, I was trying to be cute about this and take a bit of a shortcut, but I clearly broke it again at some point. Will take a look
export const getReplaceInnerSectionStyles = (theme: Theme) => css` | ||
display: flex; | ||
align-items: center; | ||
padding: 8px 10px 8px 44px; |
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.
nit: tokens?
@stephl3 note since last review - We've removed the "All" search. It honestly isn't clear what that does even on the default codemirror find and replace. I've personally never seen that option in an IDE, so we just removed it. It's been removed from the designs as well |
These last changes broke the tests - looking into that now |
<div className={findOptionsContainerStyles}> | ||
{searchString && ( | ||
<Body> | ||
{selectedIndex ?? '?'}/{findCount} |
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.
I'm picking up some unexpected behavior with selectedIndex
selection/rendering. A couple paths to try:
- When I type in a query, I see a
"?"
as the numerator which makes me think we're missing asetSelectedIndex
call somewhere. This could be expected based on the initial specs, but I wanted to point out how I see Cursor handle this which is to use the last position of the text cursor to find the next result and use its index as the numerator - When I type in a query with a match and click down arrow, the
selectedIndex
correctly identifies a result. When I then update the query without a match, theselectedIndex
does not update which will show something like"1/0"
or"2/0"
. This latter one seems like something we'd want to patch before shipping
const INPUT_WIDTH = 240; | ||
const INPUT_MIN_WIDTH = 120; | ||
|
||
const getBaseContainerStyles = ( |
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.
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.
maybe hard to tell; check out the right border
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.
I agree that it looks weird, but it is technically correct. The border is on the input and not on the search panel. The search panel has box shadows on the left and bottom, but not on the right. So what we're seeing there is the the border stopping since that's on the input, and the search panel overflowing the input.
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.
Makes sense it's technically correct, but do we want to add some additional border for that stretch? It may never be observable since users are unlikely to use the search panel when there's only 1 line of code, but I figured I would mention as design feedback in case we want to add styling
✍️ Proposed changes
CodeEditor
component. Contains all of the same functionality that was in the built in panel but matches the LG design language.🎟 Jira ticket: LG-5098
✅ Checklist
For bug fixes, new features & breaking changes
pnpm changeset
and documented my changes🧪 How to test changes