LG-5067: Create basic CodeEditor component/package #2858
Merged
Conversation
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
🦋 Changeset detectedLatest commit: ee82832 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 |
tsck
commented
May 16, 2025
Comment on lines
+7
to
+12
| global.MutationObserver = jest.fn().mockImplementation(() => ({ | ||
| observe: jest.fn(), | ||
| unobserve: jest.fn(), | ||
| disconnect: jest.fn(), | ||
| takeRecords: jest.fn().mockReturnValue([]), | ||
| })); |
There was a problem hiding this comment.
Hoping to figure out a way to do this in the render utility. Right now consumers would need to do it themselves, which isn't ideal. I can't just move it because it relies on jest and I can't use jest outside of a spec file. Open to ideas, if not will be bringing up in a future dev sync.
tsck
commented
May 16, 2025
Comment on lines
10
to
36
| export type CodeMirrorExtension = Extension; | ||
| export type CodeMirrorRef = ReactCodeMirrorRef; | ||
| export type CodeMirrorState = EditorState; | ||
| export type CodeMirrorView = EditorView; |
There was a problem hiding this comment.
Trying something new with this. CodeMirror has a view key types that we use and that will be usable by the consumer. I'm mapping them to more uniform names and exposing them. If this seems undesirable for some reason or another pattern exists for this, please let me know.
There was a problem hiding this comment.
LGTM! One thing I noticed is that code mirror has docs for the types that aren't forwarded/inherited when they're reassigned in our code. I think it would be valuable for consumers if we included tsdocs to each of these
- Can we add a
{@link ...}to the code mirror docs? - Also wondering about adding a direct copy/paste of the tsdoc associated with
Extension,ReactCodeMirrorRef, etc. Overall, I think it would be helpful although I worry that it could desync if/when we bump the dependency
There was a problem hiding this comment.
Makes sense to me! I'll update this with both suggestions shortly.
tsck
commented
May 16, 2025
There was a problem hiding this comment.
These utilities are not in alignment with our current test harness API's. I believe they're just different since they're mostly helping do things with CodeMirror internals, which is why I went in a different direction that I thought would be more helpful in this case. I've mapped it more to the RTL API, hoping that the familiarity of that will make it very easy to work with as well.
That said, if there's opinions against it, I'm more than happy to discuss and this could easily be changed to individual functions. Please let me know!
tsck
requested review from
TheSonOfThomp,
a team and
stephl3
and removed request for
a team and
TheSonOfThomp
|
Size Change: 0 B Total Size: 1.86 MB ℹ️ View Unchanged
|
stephl3
reviewed
May 19, 2025
Comment on lines
+2
to
+12
| '@leafygreen-ui/code-editor': minor | ||
| --- | ||
|
|
||
| - Creates package. | ||
| - Adds `CodeEditor` component with the following functionality: | ||
| - Basic setup | ||
| - Line wrapping | ||
| - Hyperlink support | ||
| - Forced parsing | ||
| - Placeholders | ||
| - Adds `renderEditor` test utility which exposes a number of helper methods. |
There was a problem hiding this comment.
I think this is redundant? I believe this will lead to a republish of the same source code in 0.1.0 and 0.2.0
Also, is the plan to do incremental releases or did you want to wrap it all up into an integration branch?
There was a problem hiding this comment.
Oh I see. So on the init package commit we typically don't have a changeset at all? Curious if it would be better then to bump the package down to 0.0.0?
I was just going to do incremental releases but if the team prefers integration branches I'm ok with that as well. What's the benefit of doing an integration branch here?
There was a problem hiding this comment.
My understanding is that publishing v0.1.0 and using incremental releases can be useful for consumers that need immediate access. Thinking some more on it, I'm not sure it would make sense to allow this to release until it's ready to be consumed which might be after designs/styling are complete?
There was a problem hiding this comment.
I think where I'm not following is if something is a sub-v1 release, that indicates to consumers that there's substantial risk in using it. That to me is a beta release by definition. So I'm not sure I understand why an integration branch would be necessary here. I could see that being useful when something is already versioned. For example, if this was already a v1 and the work in this epic would bring it to v2, an integration branch would be necessary since incomplete work would minor bump the v1 version, and indicate to consumers that its ready when its not. However, for sub-v1 releases, I'm not sure I feel that applies.
There was a problem hiding this comment.
Ok, spoke this one through with the team and am going to rebase this to an integration branch to mitigate the concerns that this isn't styled yet. Thanks for bringing this up!
Comment on lines
+27
to
+29
| ## Component | ||
|
|
||
| ### `<CodeEditor>` |
There was a problem hiding this comment.
is this a stub for the future or can this be removed?
There was a problem hiding this comment.
Nope, that's the component name that's being documented here.
packages/code-editor/README.md
Outdated
|
|
||
| #### Example | ||
|
|
||
| ```ts |
There was a problem hiding this comment.
Suggested change
| ```ts | |
| ```tsx |
packages/code-editor/README.md
Outdated
| ```ts | ||
| import { CodeEditor } from '@leafygreen-ui/code-editor'; | ||
|
|
||
| <CodeEditor />; |
There was a problem hiding this comment.
anything else you want to add for basic usage? maybe a code sample?
There was a problem hiding this comment.
I see this growing significantly. I considered just doing <CodeEditor {...props} /> but it just didn't seem to add much. So you'd prefer to see a default value here? I can add that. Will push shortly!
Comment on lines
10
to
36
| export type CodeMirrorExtension = Extension; | ||
| export type CodeMirrorRef = ReactCodeMirrorRef; | ||
| export type CodeMirrorState = EditorState; | ||
| export type CodeMirrorView = EditorView; |
There was a problem hiding this comment.
LGTM! One thing I noticed is that code mirror has docs for the types that aren't forwarded/inherited when they're reassigned in our code. I think it would be valuable for consumers if we included tsdocs to each of these
- Can we add a
{@link ...}to the code mirror docs? - Also wondering about adding a direct copy/paste of the tsdoc associated with
Extension,ReactCodeMirrorRef, etc. Overall, I think it would be helpful although I worry that it could desync if/when we bump the dependency
| export type CodeEditorSelectors = | ||
| (typeof CodeEditorSelectors)[keyof typeof CodeEditorSelectors]; | ||
|
|
||
| export interface CodeEditorProps extends DarkModeProps { |
There was a problem hiding this comment.
nit: can we add line breaks between props?
| return ( | ||
| <CodeMirror | ||
| value={value} | ||
| height="200px" |
There was a problem hiding this comment.
This looks like a magic number that is worth breaking out into a separate constant
There was a problem hiding this comment.
Good point! Will update
Comment on lines
+37
to
+52
| const [value, setValue] = useState(defaultValue || ''); | ||
| const editorRef = useRef<CodeMirrorRef>(null); | ||
|
|
||
| const onChange = useCallback( | ||
| (val: string) => { | ||
| setValue(val); | ||
|
|
||
| if (onChangeProp) { | ||
| onChangeProp(val); | ||
| } | ||
| }, | ||
| [onChangeProp], | ||
| ); |
There was a problem hiding this comment.
I haven't had a need for it yet myself, but useControlledValue may be handy here (or in a future refactor)
There was a problem hiding this comment.
So this is a bit odd but something I realized when putting this together. This isn't actually a controlled value. This has to do with how the CodeMirror component works. Though it accepts a value, manually updating that value doesn't appear to override the value displayed in the editor. So really it isn't controlled but accepts an initial value.
We can force controlled behavior though if we'd prefer that.
There was a problem hiding this comment.
I don't think useControlledValue forces controlled behavior. It abstracts logic for the pattern of having a value and state setter function. Looks like it could work here
| ).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('Fold gutter does not renders when disabled', () => { |
There was a problem hiding this comment.
Suggested change
| test('Fold gutter does not renders when disabled', () => { | |
| test('Fold gutter does not render when disabled', () => { |
| width="100%" | ||
| onChange={onChange} | ||
| onCreateEditor={onCreateEditor} | ||
| readOnly={readOnly} |
There was a problem hiding this comment.
I know styling isn't covered here but will leave a note for the future: I might expect readOnly state to prevent cursor blinking in addition to disabling typing
There was a problem hiding this comment.
This makes sense to me! I'm not going to update on this PR but will definitely make sure this is addressed when we get to styling things.
stephl3
approved these changes
May 22, 2025
…uration, and basic components
…or better customization
… dynamic configuration
… and new assertions
…onsistency and update related tests
…improved editor usability
…ility and update selectors
…r improved clarity
…s for better organization
…eEditor example refactor: extract CodeMirror dimensions into constants for better maintainability docs: enhance CodeMirror types documentation with detailed descriptions chore: remove unused package from ALL_PACKAGES.ts
tsck
force-pushed
the
LG-5067/editor-basic
branch
from
190a3ff to
40e1508
Compare
* chore(package): update node engine version to >= 18.22.0 * chore(package): update node engine version to >= 18.20.8
tsck
force-pushed
the
LG-5067/editor-basic
branch
from
c71c574 to
d2d186a
Compare
fix(button): remove exports field from package.json
tsck
added a commit
that referenced
this pull request
May 23, 2025
* feat(code-editor): initialize code editor package with README, configuration, and basic components * feat(code-editor): enhance CodeEditor component with new props and dependencies * refactor(code-editor): change import statements to use 'type' for CodeEditorProps * feat(code-editor): update CodeEditor stories with args and argTypes for better customization * Start test suite * Fix deps * feat(code-editor): refactor extensions handling using Compartment for dynamic configuration * feat(tests): enhance CodeEditor tests with improved selector handling and new assertions * feat(code-editor): rename 'value' prop to 'initialValue' for consistency and clarity * feat(code-editor): rename 'initialValue' prop to 'defaultValue' for consistency and update related tests * feat(code-editor): enable active line highlighting based on prop for improved editor usability * feat(tests): enable forceParsing test and update mock implementation for consistency * feat(tests): refactor CodeEditor tests to use new renderCodeEditor utility and update selectors * refactor(tests): move MutationObserver mock to testUtils for better organization * feat(docs): update README to include new CodeEditor properties and test utilities * refactor(tests): update typing test to use userEvent and export editor utilities * refactor(tests): update CodeEditor interactions and test utilities for improved clarity * test: update forceParsing method test to remove async and improve clarity * changeset * feat(docs): add CodeEditor package information to README * feat: expand exports in index.ts to include additional CodeEditor types and functions * refactor(tests): move renderCodeEditor utility to CodeEditor.testUtils for better organization * feat(tests): add TestUtils for improved test rendering utilities * fix: correct rendering description in tests and update README for CodeEditor example refactor: extract CodeMirror dimensions into constants for better maintainability docs: enhance CodeMirror types documentation with detailed descriptions chore: remove unused package from ALL_PACKAGES.ts * docs(code-editor): update README with CodeEditor example and default value * chore(package): update node engine version to >= 18.20.8 (#2868) * chore(package): update node engine version to >= 18.22.0 * chore(package): update node engine version to >= 18.20.8 * fix(package): update main and types paths in package.json * chore(package): add @lg-tools/build to devDependencies * chore(package): update build script in package.json * feat(button): add exports field for module resolution * feat(code-editor): add exports field for module resolution fix(button): remove exports field from package.json
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.
✍️ Proposed changes
CodeEditorcomponent with the following functionality:renderEditortest utility which exposes a number of helper methods, with the idea that these will get added onto as we go alone in development.🎟 Jira ticket: LG-5067
✅ Checklist
For bug fixes, new features & breaking changes
pnpm changesetand documented my changesFor new components
🧪 How to test changes
Screenshot