feat(API): add setPreviewVisible and changeSplitModeEnabled in API#1153
Open
ReFFaT wants to merge 1 commit into
Open
feat(API): add setPreviewVisible and changeSplitModeEnabled in API#1153ReFFaT wants to merge 1 commit into
ReFFaT wants to merge 1 commit into
Conversation
Reviewer's GuideAdds programmatic control over markup preview and split mode to the public Markdown editor API, moves preview visibility state into EditorImpl, introduces a change-preview-visible event, and wires the new behaviour into the React view, tests, and demo playground. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
MarkdownEditorView, theuseEffectthat focusesdivRefdepends oneditor.previewVisible, which is a property read off the instance; this will both upset exhaustive-deps linters and can be misleading for readers—consider derivingshowPreviewonce (as you already do) and using that in the dependency array instead of accessingeditor.previewVisibledirectly. - In
EditorWrapper’suseKeyfor submit, the dependency array still includesshowPrevieweven though the callback no longer references it; trimming unused dependencies here (and in other hooks where the deps no longer match the callback body after this refactor) will make the hooks’ behavior and intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MarkdownEditorView`, the `useEffect` that focuses `divRef` depends on `editor.previewVisible`, which is a property read off the instance; this will both upset exhaustive-deps linters and can be misleading for readers—consider deriving `showPreview` once (as you already do) and using that in the dependency array instead of accessing `editor.previewVisible` directly.
- In `EditorWrapper`’s `useKey` for submit, the dependency array still includes `showPreview` even though the callback no longer references it; trimming unused dependencies here (and in other hooks where the deps no longer match the callback body after this refactor) will make the hooks’ behavior and intent clearer.
## Individual Comments
### Comment 1
<location path="demo/src/components/Playground.tsx" line_range="157" />
<code_context>
yfmMods,
} = props;
const [editorMode, setEditorMode] = useState<MarkdownEditorMode>(initialEditor ?? 'wysiwyg');
+ const [previewVisible, setPreviewVisible] = useState(false);
+ const [splitModeEnabled, setSplitModeEnabled] = useState(false);
const [mdRaw, setMdRaw] = useState<MarkupString>(initial || '');
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the duplicated React state for preview and split mode and rely on mdEditor as the single source of truth for these flags.
You can drop the mirrored React state and derive everything directly from `mdEditor`, keeping logging while reducing complexity and desync risk.
### 1. Remove duplicated React state
```ts
// remove these
- const [previewVisible, setPreviewVisible] = useState(false);
- const [splitModeEnabled, setSplitModeEnabled] = useState(false);
```
### 2. Keep listeners for logging only
```ts
function onChangeSplitModeEnabled({splitModeEnabled}: {splitModeEnabled: boolean}) {
props.onChangeSplitModeEnabled?.(splitModeEnabled);
console.info(`Split mode enabled: ${splitModeEnabled}`);
}
function onChangePreviewVisible({visible}: {visible: boolean}) {
console.info(`Preview visible: ${visible}`);
}
```
No need to update React state here; `mdEditor` is the single source of truth.
### 3. Read state from `mdEditor` in the dropdown
Use `mdEditor.previewVisible` / `mdEditor.splitModeEnabled` for labels and toggling:
```tsx
{editorMode === 'markup' && (
<DropdownMenu.Item
text={`Toggle Preview (${mdEditor.previewVisible ? 'on' : 'off'})`}
action={() => {
mdEditor.setPreviewVisible({
visible: !mdEditor.previewVisible,
});
}}
/>
)}
{editorMode === 'markup' && (
<DropdownMenu.Item
text={`Toggle Split Mode (${mdEditor.splitModeEnabled ? 'on' : 'off'})`}
action={() => {
mdEditor.changeSplitModeEnabled({
splitModeEnabled: !mdEditor.splitModeEnabled,
});
}}
/>
)}
```
This keeps the behavior (including logging and the dropdown labels) while:
- Eliminating duplicated local state.
- Avoiding sync handlers whose only job was to mirror editor state into React.
- Ensuring there’s a single source of truth (`EditorImpl`) for these flags.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| yfmMods, | ||
| } = props; | ||
| const [editorMode, setEditorMode] = useState<MarkdownEditorMode>(initialEditor ?? 'wysiwyg'); | ||
| const [previewVisible, setPreviewVisible] = useState(false); |
There was a problem hiding this comment.
issue (complexity): Consider removing the duplicated React state for preview and split mode and rely on mdEditor as the single source of truth for these flags.
You can drop the mirrored React state and derive everything directly from mdEditor, keeping logging while reducing complexity and desync risk.
1. Remove duplicated React state
// remove these
- const [previewVisible, setPreviewVisible] = useState(false);
- const [splitModeEnabled, setSplitModeEnabled] = useState(false);2. Keep listeners for logging only
function onChangeSplitModeEnabled({splitModeEnabled}: {splitModeEnabled: boolean}) {
props.onChangeSplitModeEnabled?.(splitModeEnabled);
console.info(`Split mode enabled: ${splitModeEnabled}`);
}
function onChangePreviewVisible({visible}: {visible: boolean}) {
console.info(`Preview visible: ${visible}`);
}No need to update React state here; mdEditor is the single source of truth.
3. Read state from mdEditor in the dropdown
Use mdEditor.previewVisible / mdEditor.splitModeEnabled for labels and toggling:
{editorMode === 'markup' && (
<DropdownMenu.Item
text={`Toggle Preview (${mdEditor.previewVisible ? 'on' : 'off'})`}
action={() => {
mdEditor.setPreviewVisible({
visible: !mdEditor.previewVisible,
});
}}
/>
)}
{editorMode === 'markup' && (
<DropdownMenu.Item
text={`Toggle Split Mode (${mdEditor.splitModeEnabled ? 'on' : 'off'})`}
action={() => {
mdEditor.changeSplitModeEnabled({
splitModeEnabled: !mdEditor.splitModeEnabled,
});
}}
/>
)}This keeps the behavior (including logging and the dropdown labels) while:
- Eliminating duplicated local state.
- Avoiding sync handlers whose only job was to mirror editor state into React.
- Ensuring there’s a single source of truth (
EditorImpl) for these flags.
|
|
|
|
ReFFaT
force-pushed
the
feat-API-add-methods
branch
from
e707ef6 to
4534bb5
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Moves markup preview state out of React component into EditorImpl and exposes programmatic control via the public MarkdownEditorInstance interface.
What's new
previewVisible: boolean — read current preview state
setPreviewVisible(visible?: boolean) — show/hide/toggle full-screen preview; auto-disables split mode
splitModeEnabled: boolean — read current split-mode state
changeSplitModeEnabled(opts) — moved to public interface; auto-hides preview when enabled
change-preview-visible event — emitted on every visibility change
Internal
Removed useBooleanState for showPreview from MarkdownEditorView; state is now owned by EditorImpl
Added unit tests for both methods covering toggle, mutual exclusion, and no-op behaviour
Demo
Added "Toggle Preview" and "Toggle Split Mode" buttons to Playground (markup mode only).
Summary by Sourcery
Expose programmatic control of full-screen markup preview and split mode via the Markdown editor API and centralize preview state in the core editor implementation.
New Features:
Enhancements:
Tests: