Add maxHeight toggle to CodeMirror Demo + Better demo docs showing diff algorithm bugs
#2364
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Bundle ReportChanges will increase total bundle size by 1.94kB (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- app/controllers/demo/code-mirror.ts (4 hunks)
- app/routes/demo/code-mirror.ts (1 hunks)
- app/templates/demo/code-mirror.hbs (2 hunks)
🔇 Additional comments (6)
app/routes/demo/code-mirror.ts (1)
30-30: LGTM! Verify integration with related components.The addition of 'maxHeight' to QUERY_PARAMS is well-implemented and maintains consistency with the existing pattern.
Let's verify the integration with related components:
✅ Verification successful
Integration verified - maxHeight parameter is properly implemented across components
The verification confirms that:
- Controller has the maxHeight property defined in OPTION_DEFAULTS and tracked for reactivity
- Template includes both the checkbox toggle UI and applies the 'max-h-96' class conditionally
- The implementation is consistent across route, controller, and template components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the maxHeight parameter integration across components # Test 1: Verify controller handles the maxHeight parameter echo "Checking controller implementation..." rg -A 5 "maxHeight" "app/controllers/demo/code-mirror.ts" # Test 2: Verify template includes maxHeight toggle echo "Checking template implementation..." rg -A 5 "maxHeight" "app/templates/demo/code-mirror.hbs"Length of output: 1284
app/controllers/demo/code-mirror.ts (4)
56-58: LGTM: Example document changes effectively demonstrate diff behavior.The modifications to the example text document, including new line insertions and text modifications, provide clear test cases for visualizing the CodeMirror diff algorithm's behavior.
133-133: LGTM: maxHeight option properly configured.The maxHeight option is correctly added to OPTION_DEFAULTS with a sensible default value of true.
187-187: LGTM: maxHeight properly added to queryParams.The maxHeight parameter is correctly added to queryParams array, maintaining alphabetical order and enabling URL persistence of the editor configuration.
239-239: LGTM: maxHeight tracked property properly implemented.The maxHeight property is correctly implemented as a tracked property and initialized with the default value.
app/templates/demo/code-mirror.hbs (1)
179-184: LGTM: Well-structured maxHeight toggle implementationThe new maxHeight toggle follows the established pattern and maintains consistency with other option controls. The tooltip provides clear user guidance.
| class="block overflow-auto mt-4 | ||
| {{if this.maxHeight 'max-h-96'}} | ||
| {{if this.outline 'outline outline-dotted outline-gray-200 dark:outline-gray-700'}}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the maxHeight value configurable
While the implementation works, the hard-coded max-h-96 (24rem/384px) might be too restrictive for some use cases. Consider making this configurable similar to other numeric options like tabSize or unchangedMargin.
Example implementation:
- {{if this.maxHeight 'max-h-96'}}
+ {{if this.maxHeight (concat 'max-h-' this.selectedMaxHeight)}}With corresponding controller changes:
maxHeights: ['72', '96', '128'], // 18rem, 24rem, 32rem
selectedMaxHeightIndex: 1, // default to 96 (24rem)There was a problem hiding this comment.
Great idea, considered it too, but not very relevant for the context of the demo page, at the moment. Plus too much additional work: adding a select, checkbox for select, getter for selected index -> value conversion, handling selected index change, and adding both select + checkbox to the query params.
There was a problem hiding this comment.
@VasylMarchuk, understood. Thank you for the clarification.
(_/)
(•_•)
💔
✏️ Learnings added
Learnt from: VasylMarchuk
PR: codecrafters-io/frontend#2364
File: app/templates/demo/code-mirror.hbs:351-353
Timestamp: 2024-10-27T10:18:53.775Z
Learning: In the context of the demo page, making `maxHeight` configurable is not necessary due to additional complexity and is not relevant at the moment.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files📢 Thoughts on this report? Let us know! |
VasylMarchuk
changed the title
maxHeight toggle to CodeMirror Demo + Better demo doc showing diff algorithm bugsmaxHeight toggle to CodeMirror Demo + Better demo docs showing diff algorithm bugs
VasylMarchuk
force-pushed
the
code-mirror/better-text-diff-example
branch
from
6d3625d to
f392d07
Compare
Related to #1231
Brief
This adds a toggle
maxHeightto the CodeMirror's demo page, which allows limiting (or not) of the editor's height.Details
Modified the demo text & SQL documents to better showcase the problems of CodeMirror's diff algorithm.
Video + Screenshot
2024-10-27.12.04.56.mov
Checklist
[percy]in the message to trigger)Summary by CodeRabbit
Release Notes
New Features
Improvements
maxHeight, to enhance the controller's configuration options.