refactor!: remove Client.isBrowserSupported
#679
Conversation
BREAKING CHANGES: This function didn't correctly validate all prerequisites require to know if the browser supports maxGraph. So, remove it without replacement.
WalkthroughThis pull request updates documentation and internal code by restricting and removing several utility functions and browser support checks. The internal functions Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DropHandler as Drop Event Handler
participant Graph
User->>DropHandler: Drop file event
DropHandler->>Graph: Convert image to data URL and insert at coordinates
Graph-->>DropHandler: Confirm insertion
DropHandler-->>User: Update graph display
Possibly related PRs
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/html/stories/Drop.stories.js (1)
72-95: Consider documenting browser requirements.With the removal of browser support checks, it would be helpful to document the required browser capabilities (File API, FileReader, FileList) in the component's documentation or README.
CHANGELOG.md (1)
23-24: Fix grammatical issue in the changelog entry.There's a minor grammatical issue in the text. The word "require" should be "required".
Apply this diff to fix the grammar:
-`Client.isBrowserSupported` has been removed. It didn't correctly validate all prerequisites require to know if the browser supports maxGraph. +`Client.isBrowserSupported` has been removed. It didn't correctly validate all prerequisites required to know if the browser supports maxGraph.🧰 Tools
🪛 LanguageTool
[grammar] ~23-~23: Did you mean “knowing”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...ctly validate all prerequisites require to know if the browser supports maxGraph. So, r...(ALLOW_TO)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)packages/core/src/Client.ts(0 hunks)packages/html/stories/Drop.stories.js(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/Client.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~23-~23: Did you mean “knowing”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...ctly validate all prerequisites require to know if the browser supports maxGraph. So, r...
(ALLOW_TO)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-2022)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (4)
packages/html/stories/Drop.stories.js (4)
56-58: LGTM!The context menu disabling logic is clear and correctly implemented.
59-61: LGTM!The graph creation is correctly implemented.
62-64: LGTM!The rubberband selection is correctly implemented.
65-70: LGTM!The dragover event handling is correctly implemented.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/html/stories/Drop.stories.js (1)
99-184: Consider modernizing the code.A few suggestions to improve the code:
- The IE11-specific SVG handling might be unnecessary as IE11 is end-of-life.
- Replace
atob/btoawith more robust Base64 encoding/decoding to handle Unicode characters correctly.Here's how you could modernize the code:
- data = `data:image/svg+xml,${btoa(xmlUtils.getXml(svgs[0], '\n'))}`; + data = `data:image/svg+xml,${encodeURIComponent(xmlUtils.getXml(svgs[0], '\n'))}`;Also, consider adding error handling for the image loading:
img.onload = () => { const w = Math.max(1, img.width); const h = Math.max(1, img.height); // ... }; +img.onerror = (error) => { + console.error('Failed to load image:', error); +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)packages/html/stories/Drop.stories.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🔇 Additional comments (2)
packages/html/stories/Drop.stories.js (2)
55-62: LGTM!The initialization code is clean and properly configurable through args.
64-69: LGTM!The dragover event handler follows standard drag-and-drop practices.
BREAKING CHANGES: This function didn't correctly validate all prerequisites require to know if the browser supports
maxGraph. So, remove it without replacement.
Summary by CodeRabbit
isBrowserSupportedmethod, which previously provided unreliable browser support validation.