feat(code-editor): expand to fullscreen view #3680
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds an expandable/fullscreen mode to the code editor. Introduces a fullscreen button, dialog-based rendering, DOM reparenting of the CodeMirror instance between inline and dialog containers, new public prop and event, related styles, and an example demonstrating the feature. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CE as CodeEditor (component)
participant CM as CodeMirror Instance
participant BTN as Fullscreen Button
participant DLG as limel-dialog
Note over CE: Initial render (expandable = true)
CE->>CM: Initialize once
CE->>CE: Mount CM into inline container
User->>BTN: Click expand
BTN-->>CE: expand event
CE->>DLG: Open dialog (dialogOpen = true)
CE->>CM: Reparent CM into dialog container
CM-->>CE: Refresh/layout
alt Close dialog
User->>DLG: Close
DLG-->>CE: close event
CE->>CE: dialogOpen = false
CE->>CM: Reparent CM back to inline
CM-->>CE: Refresh/layout
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3680/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/components/code-editor/code-editor.scss(3 hunks)src/components/code-editor/code-editor.tsx(5 hunks)src/components/code-editor/examples/code-editor-expandable.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/wrap-multiple-jsx-elements-in-host-not-in-array.mdc)
When returning multiple JSX elements from the
rendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element. When there is already a single top-level element, it does not have to be wrapped in<Host>.
Files:
src/components/code-editor/examples/code-editor-expandable.tsxsrc/components/code-editor/code-editor.tsx
⚙️ CodeRabbit configuration file
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
Files:
src/components/code-editor/examples/code-editor-expandable.tsxsrc/components/code-editor/code-editor.tsx
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
Files:
src/components/code-editor/examples/code-editor-expandable.tsxsrc/components/code-editor/code-editor.tsx
**/*.{tsx,scss}
⚙️ CodeRabbit configuration file
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
Files:
src/components/code-editor/examples/code-editor-expandable.tsxsrc/components/code-editor/code-editor.scsssrc/components/code-editor/code-editor.tsx
src/components/**/examples/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publicly exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
Files:
src/components/code-editor/examples/code-editor-expandable.tsx
src/components/**/*.tsx
⚙️ CodeRabbit configuration file
src/components/**/*.tsx: When contributors add new props to existing components in the lime-elements repository, they should always add documentation examples that demonstrate the new prop's usage and explain how it works. This helps with user adoption, feature discoverability, and maintains comprehensive documentation.
Files:
src/components/code-editor/examples/code-editor-expandable.tsxsrc/components/code-editor/code-editor.tsx
src/components/**/*.scss
📄 CodeRabbit inference engine (.cursor/rules/css-class-names-in-components-using-shadow-dom.mdc)
Do not use BEM-style class names in CSS for components, as components use shadow-DOM.
Files:
src/components/code-editor/code-editor.scss
🧬 Code graph analysis (1)
src/components/code-editor/examples/code-editor-expandable.tsx (1)
src/components/code-editor/code-editor.tsx (1)
Component(29-340)
🪛 Biome (2.1.2)
src/components/code-editor/code-editor.tsx
[error] 298-305: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 316-323: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 307-307: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docs / Publish Docs
🔇 Additional comments (5)
src/components/code-editor/examples/code-editor-expandable.tsx (1)
24-35: Good, minimal example demonstrating the newexpandablepropShows JSON mode, wiring
onChange, and enablesexpandable. Clear and aligned with guidelines for documenting new props.src/components/code-editor/code-editor.scss (2)
123-128: Host positioning for overlay control is correctAdding
position: relativeto:hostis necessary for the absolutely-positioned fullscreen button.
135-149: Inline host wrapper and readonly cursor visibility look goodThe
.editor-inline-hostwrapper aligns with DOM reparenting, and restoring cursor visibility under.editor.readonlyis appropriate.src/components/code-editor/code-editor.tsx (2)
77-82: New public API (expandableprop andexpandevent) — LGTMAPI surface is small and sensible; naming is clear. Example coverage is included.
Also applies to: 90-95
298-305: Biome a11y warnings look like false positives for custom elementsThe “Static Elements should not be interactive” warnings are likely misfires against the
limel-icon-buttoncustom element. After the<Host>refactor, the “missing key” warning is resolved. No code change needed for a11y here.If Biome supports it, consider configuring the a11y rule to recognize custom elements as interactive to avoid future noise.
Also applies to: 316-323
| .code-fullscreen-button { | ||
| position: absolute; | ||
| top: 0.4rem; | ||
| right: 0.4rem; | ||
| z-index: 100; | ||
| opacity: 0; | ||
| pointer-events: none; | ||
| transition: opacity 0.15s ease; | ||
| } | ||
| .code-fullscreen-button-focus:focus, | ||
| .code-fullscreen-button-focus:focus-visible, | ||
| .code-fullscreen-button-focus:active { | ||
| opacity: 1; | ||
| pointer-events: auto; | ||
| } | ||
| :host(:focus-within) .code-fullscreen-button-focus { | ||
| opacity: 1 !important; | ||
| pointer-events: auto !important; | ||
| } | ||
| limel-dialog { | ||
| --code-editor-max-height: 100%; | ||
| .code-fullscreen-button { | ||
| opacity: 1; | ||
| pointer-events: auto; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fullscreen editor may not stretch to full height in dialog
--code-editor-max-height: 100% sets only a max-height on the scroll area. Without explicit heights on the dialog wrappers, the editor might not fill the viewport.
Apply this diff to ensure full-height layout in fullscreen:
limel-dialog {
--code-editor-max-height: 100%;
+ height: 100%;
+ .editor-dialog-wrapper,
+ .editor-dialog-host {
+ height: 100%;
+ }
+ .cm-s-lime.CodeMirror {
+ height: 100%;
+ }
.code-fullscreen-button {
opacity: 1;
pointer-events: auto;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .code-fullscreen-button { | |
| position: absolute; | |
| top: 0.4rem; | |
| right: 0.4rem; | |
| z-index: 100; | |
| opacity: 0; | |
| pointer-events: none; | |
| transition: opacity 0.15s ease; | |
| } | |
| .code-fullscreen-button-focus:focus, | |
| .code-fullscreen-button-focus:focus-visible, | |
| .code-fullscreen-button-focus:active { | |
| opacity: 1; | |
| pointer-events: auto; | |
| } | |
| :host(:focus-within) .code-fullscreen-button-focus { | |
| opacity: 1 !important; | |
| pointer-events: auto !important; | |
| } | |
| limel-dialog { | |
| --code-editor-max-height: 100%; | |
| .code-fullscreen-button { | |
| opacity: 1; | |
| pointer-events: auto; | |
| } | |
| } | |
| .code-fullscreen-button { | |
| position: absolute; | |
| top: 0.4rem; | |
| right: 0.4rem; | |
| z-index: 100; | |
| opacity: 0; | |
| pointer-events: none; | |
| transition: opacity 0.15s ease; | |
| } | |
| .code-fullscreen-button-focus:focus, | |
| .code-fullscreen-button-focus:focus-visible, | |
| .code-fullscreen-button-focus:active { | |
| opacity: 1; | |
| pointer-events: auto; | |
| } | |
| :host(:focus-within) .code-fullscreen-button-focus { | |
| opacity: 1 !important; | |
| pointer-events: auto !important; | |
| } | |
| limel-dialog { | |
| --code-editor-max-height: 100%; | |
| height: 100%; | |
| .editor-dialog-wrapper, | |
| .editor-dialog-host { | |
| height: 100%; | |
| } | |
| .cm-s-lime.CodeMirror { | |
| height: 100%; | |
| } | |
| .code-fullscreen-button { | |
| opacity: 1; | |
| pointer-events: auto; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/components/code-editor/code-editor.scss around lines 265 to 290, the
dialog CSS only sets --code-editor-max-height: 100% which doesn't force the
dialog and its internal wrappers to fill the viewport; update styles to
explicitly set height: 100% (or min-height: 100vh where appropriate) on the
limel-dialog, its content/wrapper elements and the editor host/scroll container
so the fullscreen editor can expand to full height; ensure the scroll area uses
height: 100% instead of only max-height and keep the existing
opacity/pointer-events rules intact.
There was a problem hiding this comment.
The height should only fill the height of the dialog (which is fullscreen), we should avoid another wrapper container.
| @State() | ||
| private dialogOpen: boolean = false; | ||
|
|
||
| private handleClickExpand = () => { | ||
| if (!this.expandable) { | ||
| return; | ||
| } | ||
| this.expand.emit(); | ||
| this.dialogOpen = true; | ||
| }; | ||
|
|
||
| private handleCloseDialog = () => { | ||
| this.dialogOpen = false; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Restore focus after closing dialog
Improve keyboard/a11y by returning focus to the control that opened fullscreen.
Apply this diff:
export class CodeEditor {
@@
- private dialogOpen: boolean = false;
+ private dialogOpen: boolean = false;
+ private previouslyFocused: HTMLElement;
@@
private handleClickExpand = () => {
if (!this.expandable) {
return;
}
this.expand.emit();
+ this.previouslyFocused =
+ (this.host.shadowRoot?.activeElement as HTMLElement) ||
+ (document.activeElement as HTMLElement);
this.dialogOpen = true;
};
@@
private handleCloseDialog = () => {
this.dialogOpen = false;
+ // Restore focus for accessibility
+ this.previouslyFocused?.focus();
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/code-editor/code-editor.tsx around lines 96 to 110, the
dialogOpen toggle opens a fullscreen dialog but does not restore keyboard focus
to the control that opened it; capture the element that triggered
handleClickExpand (e.g., store document.activeElement in a private field like
lastActiveElement before setting dialogOpen=true) and on handleCloseDialog call
focus() on that stored element (with null checks and a try/catch or instanceof
HTMLElement guard), then clear the stored reference; this ensures focus returns
to the opener for keyboard/a11y.
There was a problem hiding this comment.
We can assume that the user doesn't want to edit in the small view, once done in the expanded fullscreen editor. To serve keyboard navigation, more code than above needs to be added to recreate the expand button and refocusing the editor. Can be considered another time if necessary.
| public componentDidRender() { | ||
| if (this.editor) { | ||
| if (!this.editor) { | ||
| this.editor = this.createEditor(); | ||
| this.cmWrapper = this.editor.getWrapperElement() as HTMLElement; | ||
| } | ||
| if (!this.expandable) { | ||
| return; | ||
| } | ||
|
|
||
| this.editor = this.createEditor(); | ||
| // Re-parent the existing CodeMirror DOM into dialog or back inline | ||
| if (this.dialogOpen) { | ||
| const dialogHost = this.host.shadowRoot.querySelector( | ||
| '.editor-dialog-host' | ||
| ) as HTMLElement; | ||
| if (dialogHost && this.cmWrapper?.parentElement !== dialogHost) { | ||
| dialogHost.append(this.cmWrapper); | ||
| requestAnimationFrame(() => this.editor.refresh()); | ||
| } | ||
| } else { | ||
| const inlineHost = this.host.shadowRoot.querySelector( | ||
| '.editor-inline-host .editor' | ||
| ) as HTMLElement; | ||
| if (inlineHost && this.cmWrapper?.parentElement !== inlineHost) { | ||
| inlineHost.append(this.cmWrapper); | ||
| requestAnimationFrame(() => this.editor.refresh()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Reparenting bug when expandable is toggled off while dialog is open
Early return on !this.expandable skips moving the editor back inline, leaving the CodeMirror DOM detached when the prop is disabled during an open dialog.
Apply this diff to always ensure the inline host contains the editor when not expandable:
public componentDidRender() {
if (!this.editor) {
this.editor = this.createEditor();
this.cmWrapper = this.editor.getWrapperElement() as HTMLElement;
}
- if (!this.expandable) {
- return;
- }
- // Re-parent the existing CodeMirror DOM into dialog or back inline
- if (this.dialogOpen) {
+ // Re-parent the existing CodeMirror DOM into dialog or back inline
+ const inlineHost = this.host.shadowRoot.querySelector(
+ '.editor-inline-host .editor'
+ ) as HTMLElement;
+ if (!this.expandable || !this.dialogOpen) {
+ if (inlineHost && this.cmWrapper?.parentElement !== inlineHost) {
+ inlineHost.append(this.cmWrapper);
+ requestAnimationFrame(() => this.editor.refresh());
+ }
+ return;
+ }
+ if (this.dialogOpen) {
const dialogHost = this.host.shadowRoot.querySelector(
'.editor-dialog-host'
) as HTMLElement;
if (dialogHost && this.cmWrapper?.parentElement !== dialogHost) {
dialogHost.append(this.cmWrapper);
requestAnimationFrame(() => this.editor.refresh());
}
- } else {
- const inlineHost = this.host.shadowRoot.querySelector(
- '.editor-inline-host .editor'
- ) as HTMLElement;
- if (inlineHost && this.cmWrapper?.parentElement !== inlineHost) {
- inlineHost.append(this.cmWrapper);
- requestAnimationFrame(() => this.editor.refresh());
- }
}
}Optionally also close the dialog when disabled:
// @Watch('expandable')
private onExpandableChange(next: boolean) {
if (!next) this.dialogOpen = false;
}🤖 Prompt for AI Agents
In src/components/code-editor/code-editor.tsx around lines 145 to 171, the early
return when !this.expandable prevents re-parenting the CodeMirror DOM back into
the inline host if expandable is turned off while the dialog is open, leaving
the editor detached; remove or change the early return so that when
!this.expandable you still perform the "else" branch logic to find the inline
host and append this.cmWrapper (and call this.editor.refresh()), and optionally
set this.dialogOpen = false in an @Watch handler for expandable to ensure the
dialog is closed when expandability is disabled.
There was a problem hiding this comment.
It is very unlikely, that this issue could occur. If the prop ever even gets disabled, it is a rare event that we don't need to consider.
| return [ | ||
| this.expandable && !this.dialogOpen && ( | ||
| <limel-icon-button | ||
| class="code-fullscreen-button code-fullscreen-button-focus" | ||
| label="Expand" | ||
| icon="expand" | ||
| onClick={this.handleClickExpand} | ||
| elevated={true} | ||
| aria-label="Expand code editor" | ||
| /> | ||
| ), | ||
| <div class="editor-inline-host"> | ||
| <div class={classList}></div> | ||
| </div>, | ||
| this.expandable && this.dialogOpen && ( | ||
| <limel-dialog | ||
| open | ||
| fullscreen={true} | ||
| onClose={this.handleCloseDialog} | ||
| > | ||
| <limel-icon-button | ||
| class="code-fullscreen-button" | ||
| label="Collapse" | ||
| icon="collapse" | ||
| aria-label="Collapse code editor" | ||
| elevated={true} | ||
| onClick={this.handleCloseDialog} | ||
| /> | ||
| <div class="editor-dialog-wrapper"> | ||
| <div class="editor-dialog-host"></div> | ||
| </div> | ||
| </limel-dialog> | ||
| ), | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
StencilJS: wrap multiple root elements in <Host> instead of returning an array
Returning an array violates our StencilJS guideline and triggers the “missing key” linter warning. Wrap in <Host> and import it.
Apply this diff:
-import {
- Component,
- h,
- Prop,
- Element,
- Event,
- EventEmitter,
- State,
- Watch,
-} from '@stencil/core';
+import {
+ Component,
+ h,
+ Prop,
+ Element,
+ Event,
+ EventEmitter,
+ State,
+ Watch,
+ Host,
+} from '@stencil/core';
@@
- return [
- this.expandable && !this.dialogOpen && (
+ return (
+ <Host>
+ {this.expandable && !this.dialogOpen && (
<limel-icon-button
class="code-fullscreen-button code-fullscreen-button-focus"
label="Expand"
icon="expand"
onClick={this.handleClickExpand}
elevated={true}
aria-label="Expand code editor"
/>
- ),
- <div class="editor-inline-host">
- <div class={classList}></div>
- </div>,
- this.expandable && this.dialogOpen && (
+ )}
+ <div class="editor-inline-host">
+ <div class={classList}></div>
+ </div>
+ {this.expandable && this.dialogOpen && (
<limel-dialog
open
fullscreen={true}
onClose={this.handleCloseDialog}
>
<limel-icon-button
class="code-fullscreen-button"
label="Collapse"
icon="collapse"
aria-label="Collapse code editor"
elevated={true}
onClick={this.handleCloseDialog}
/>
<div class="editor-dialog-wrapper">
<div class="editor-dialog-host"></div>
</div>
</limel-dialog>
- ),
- ];
+ )}
+ </Host>
+ );Also applies to: 1-10
🧰 Tools
🪛 Biome (2.1.2)
[error] 298-305: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 316-323: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 307-307: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🤖 Prompt for AI Agents
In src/components/code-editor/code-editor.tsx around lines 296 to 329, multiple
root JSX elements are returned as an array which violates StencilJS guidelines
and triggers the "missing key" linter warning; replace the array return with a
single <Host> wrapper containing the existing elements, add an import for Host
from '@stencil/core' at the top of the file if missing, and ensure conditional
fragments stay as children of Host (no array) so the component has a single root
element.
| * Make the editor expandable to fullscreen dialog. | ||
| */ | ||
| @Prop() | ||
| public expandable: boolean = false; |
There was a problem hiding this comment.
This is a pretty nifty thing. 🤔 Maybe good to initialize this as true?
Also we don't necessarily need to write boolean when initializing to true and false
| public expandable: boolean = false; | |
| public expandable = true; |
| @State() | ||
| private dialogOpen: boolean = false; | ||
|
|
||
| private handleClickExpand = () => { |
There was a problem hiding this comment.
Not sure about this name. It should be handleExpandClick or perhaps describe better what happens when user clicks, so something like
openFullscreenMode or similar?
| this.dialogOpen = true; | ||
| }; | ||
|
|
||
| private handleCloseDialog = () => { |
There was a problem hiding this comment.
(see my previous comment) here also, the naming doesn't really say which dialog? Perhaps better to rename it to closeFullscreenMode
| this.expandable && !this.dialogOpen && ( | ||
| <limel-icon-button | ||
| class="code-fullscreen-button code-fullscreen-button-focus" | ||
| label="Expand" |
There was a problem hiding this comment.
These types of labels which are inbuilt into the component should be translated. We have a bunch of translation files in Lime Elements for these.
Also note that the expand and close buttons would need different labels
There was a problem hiding this comment.
What is our policy with labels? Do we always have them for accessibility reasons?
|
|
||
| public componentDidRender() { | ||
| if (this.editor) { | ||
| if (!this.editor) { |
There was a problem hiding this comment.
Too many conditions and it is very complex.
There was a problem hiding this comment.
Too many conditions and it is very complex.
Do you mean this condition is unnecessary or the overall structure is too complex?
Because my idea was it to move the same editor between the two places, which is more complex, but that way we could save the repetitive code of placing the code editor twice.
There was a problem hiding this comment.
No look at how many if, else statements we have in componentDidRender. We have even nested conditions on line 168 and 176. It is a bit difficult to know what is happening in this life cycle hook. Ideally, these should be split into smaller, reusable helpers. Especially the codes in if (this.openFullscreenMode) { on line 164 and in the else statement, they look pretty similar. They can be refactored
3 participants
fix: #3679
Summary by CodeRabbit
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: