refactor!: replace FONT enum by FONT_STYLE_MASK value object #804
Conversation
BREAKING CHANGES: The `constants.FONT` enum has been removed and replaced by the `constants.FONT_STYLE_FLAG` value object.
WalkthroughThe changes systematically replace the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant Constants
participant Renderer
User->>Editor: Toggle font style (e.g., bold)
Editor->>Constants: Access FONT_STYLE_MASK.BOLD
Editor->>Renderer: Apply style using FONT_STYLE_MASK.BOLD
Renderer->>Constants: Use FONT_STYLE_MASK to determine rendering
sequenceDiagram
participant TestSuite
participant Constants
TestSuite->>Constants: Import FONT_STYLE_MASK
TestSuite->>TestSuite: Use FONT_STYLE_MASK in assertions and style flag combinations
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (2)
packages/ts-example-selected-features/vite.config.js (1)
30-30: Verify the chunk size warning limit bump
ThechunkSizeWarningLimitwas incremented from 377 to 378. Please confirm this change reflects the actual bundle size increase for the@maxgraph/corechunk after your refactor. If this is intentional, consider adding a brief comment to document why the threshold was adjusted.packages/core/src/util/styleUtils.ts (1)
350-352: Update example usage in JSDoc snippet
The documentation example now demonstratesconstants.FONT_STYLE_FLAG.BOLD. For clarity, consider showing the corresponding import in the snippet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
CHANGELOG.md(1 hunks)packages/core/__tests__/internal/utils.test.ts(1 hunks)packages/core/__tests__/util/styleUtils.test.ts(3 hunks)packages/core/__tests__/view/mixins/CellMixin.test.ts(2 hunks)packages/core/src/editor/Editor.ts(2 hunks)packages/core/src/types.ts(1 hunks)packages/core/src/util/Constants.ts(1 hunks)packages/core/src/util/styleUtils.ts(3 hunks)packages/core/src/view/canvas/SvgCanvas2D.ts(5 hunks)packages/core/src/view/geometry/node/TextShape.ts(3 hunks)packages/core/src/view/plugins/CellEditorHandler.ts(2 hunks)packages/html/stories/EdgeCurvedAndRounded.stories.ts(1 hunks)packages/html/stories/HoverStyle.stories.js(1 hunks)packages/html/stories/Merge.stories.ts(1 hunks)packages/html/stories/SecondLabel.stories.js(1 hunks)packages/ts-example-selected-features/vite.config.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/core/__tests__/internal/utils.test.ts (2)
packages/core/src/internal/utils.ts (1)
matchBinaryMask(68-70)packages/core/src/util/Constants.ts (1)
FONT_STYLE_FLAG(402-411)
packages/core/__tests__/util/styleUtils.test.ts (3)
packages/core/src/util/Constants.ts (1)
FONT_STYLE_FLAG(402-411)packages/core/src/util/styleUtils.ts (2)
setStyleFlag(391-412)setCellStyleFlags(360-380)packages/core/src/view/mixins/CellsMixin.ts (1)
setCellStyleFlags(349-361)
packages/core/__tests__/view/mixins/CellMixin.test.ts (1)
packages/core/src/util/Constants.ts (1)
FONT_STYLE_FLAG(402-411)
packages/core/src/view/plugins/CellEditorHandler.ts (2)
packages/html/stashed/grapheditor/www/js/Graph.js (2)
bold(9508-9510)italic(9511-9513)packages/core/src/util/Constants.ts (1)
FONT_STYLE_FLAG(402-411)
packages/core/src/editor/Editor.ts (1)
packages/core/src/util/Constants.ts (1)
FONT_STYLE_FLAG(402-411)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build
- GitHub Check: build (windows-2022)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (34)
packages/core/src/util/Constants.ts (1)
402-411: Good implementation of the FONT_STYLE_FLAG value object.The conversion from an enum to a constant object with
as constis a good approach. This provides better type safety while maintaining the same functionality and bit flag values (1, 2, 4, 8) for font styles.packages/html/stories/SecondLabel.stories.js (1)
130-130: Correctly updated to use FONT_STYLE_FLAG instead of FONT.The update from
constants.FONT.ITALICtoconstants.FONT_STYLE_FLAG.ITALICpreserves the original functionality while adapting to the new API.packages/html/stories/HoverStyle.stories.js (1)
48-48: Correctly updated to use FONT_STYLE_FLAG instead of FONT.The font style value has been properly updated to use the new
FONT_STYLE_FLAG.BOLDconstant, maintaining the same functionality in the hover style logic.packages/html/stories/EdgeCurvedAndRounded.stories.ts (2)
94-94: Correctly updated to use FONT_STYLE_FLAG instead of FONT.The vertex style's font style property has been properly updated to use the new constant object.
99-99: Correctly updated to use FONT_STYLE_FLAG instead of FONT.The edge style's font style property has been properly updated to use the new constant object.
packages/html/stories/Merge.stories.ts (2)
74-74: Replaced deprecated FONT enum with FONT_STYLE_FLAG for vertex style
Consistently updated to use the new bitmask constant.
80-80: Replaced deprecated FONT enum with FONT_STYLE_FLAG for edge style
Consistently updated to use the new bitmask constant.packages/core/__tests__/internal/utils.test.ts (5)
18-18: Replaced FONT import with FONT_STYLE_FLAG
Import updated to the new value object in the test.
23-25: Updated test to use FONT_STYLE_FLAG for STRIKETHROUGH
Assertion now matches the new bitmask constant.
28-28: Updated test to use FONT_STYLE_FLAG for BOLD
Assertion now matches the new bitmask constant.
31-31: Updated test to use FONT_STYLE_FLAG for UNDERLINE
Assertion now matches the new bitmask constant.
34-34: Updated test to use FONT_STYLE_FLAG for ITALIC
Assertion now matches the new bitmask constant.packages/core/__tests__/view/mixins/CellMixin.test.ts (2)
20-20: Replaced FONT import with FONT_STYLE_FLAG
Import updated to the new value object in the mixin test.
53-53: Updated fontStyle flag for setCellStyleFlags
UsingFONT_STYLE_FLAG.UNDERLINEto compute the combined fontStyle correctly.CHANGELOG.md (1)
27-27: Document breaking change from FONT to FONT_STYLE_FLAG
CHANGELOG entry clearly describes the removal of theFONTenum and replacement byFONT_STYLE_FLAG.packages/core/src/types.ts (1)
337-338: Updated documentation to reference FONT_STYLE_FLAG
Comment now correctly links to the new constant object for font styles.packages/core/src/editor/Editor.ts (2)
36-36: Update import to useFONT_STYLE_FLAG
Import now references the newFONT_STYLE_FLAGconstant object instead of the removedFONTenum. Confirm there are no lingeringFONTimports in this module.
1143-1156: Use new font style flags in toggle actions
The action handlers for 'bold', 'italic', and 'underline' have been updated to calltoggleCellStyleFlagswithFONT_STYLE_FLAG.BOLD,.ITALIC, and.UNDERLINE. This change preserves the existing behavior under the new flag constants.packages/core/src/util/styleUtils.ts (1)
20-25: ReplaceFONTenum import withFONT_STYLE_FLAG
The import correctly brings inFONT_STYLE_FLAGfromConstants.ts. Ensure that all references to font style flags in this file now useFONT_STYLE_FLAG.packages/core/src/view/canvas/SvgCanvas2D.ts (4)
27-27: Update import to useFONT_STYLE_FLAG
The import statement has been updated from the removedFONTenum to the newFONT_STYLE_FLAGconstant object.
475-483: UseFONT_STYLE_FLAGin alternate text rendering
IngetAlternateText, the bitmask checks have been switched to use the new flags (FONT_STYLE_FLAG.BOLD, etc.), and map to the same SVG attributes as before.
1360-1368: Update font style flags in CSS generation
ThegetTextCssmethod now correctly togglesfont-weightandfont-stylebased onFONT_STYLE_FLAG. Behavior is unchanged.
1653-1661: Apply new font style flags inupdateFont
In theupdateFontmethod, bitmask checks now useFONT_STYLE_FLAGto set SVG attributes (font-weight,font-style,text-decoration). This aligns with the refactor.packages/core/src/view/plugins/CellEditorHandler.ts (3)
28-28: Consistent enum replacement with object constant.The import has been properly changed from
FONTenum to the newFONT_STYLE_FLAGvalue object, aligning with the PR objective.
699-700: Properly replaced enum with value object for bold and italic flags.The bitwise font style checks have been correctly updated to use
FONT_STYLE_FLAGinstead of the previousFONTenum.
703-707: Correctly updated underline and strikethrough font style flag checks.The bitwise operations have been properly migrated to use
FONT_STYLE_FLAGwhile maintaining the same functionality.packages/core/src/view/geometry/node/TextShape.ts (3)
26-26: Import correctly updated to use FONT_STYLE_FLAG.The import has been properly changed to use the new constant object, consistent with the PR objective.
597-606: Font style checks properly migrated in getTextCss method.The bitwise checks in the CSS generation have been correctly updated to use
FONT_STYLE_FLAGwith thematchBinaryMaskhelper function, maintaining the same functionality.
802-815: Successfully updated font style checks in updateFont method.The font style flag checks in the
updateFontmethod have been properly migrated to useFONT_STYLE_FLAGconstants.packages/core/__tests__/util/styleUtils.test.ts (5)
24-24: Updated import to use new FONT_STYLE_FLAG constant.The import has been correctly changed to use the new constant object instead of the enum.
48-54: Test case properly updated to use FONT_STYLE_FLAG.The test case has been appropriately updated to use the new
FONT_STYLE_FLAGconstant object for creating styles and setting flags.
55-56: Expectation correctly updated to use FONT_STYLE_FLAG bitwise operations.The test's expected result has been properly updated to use the new constants with the same bitwise logic.
63-68: Multiple flag combination tests correctly updated.All test cases that combine multiple font style flags have been properly migrated to use
FONT_STYLE_FLAG, maintaining the same test logic and expectations.
146-152: Successfully updated setCellStyleFlags test.The test for
setCellStyleFlagsfunction has been properly updated to use the newFONT_STYLE_FLAGconstant.
introduce variables to remove duplicated calls use matchBinaryMask for clarity and consistency with other places in the code
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/internal/utils.ts (1)
68-69: Consider handling nullish values more explicitlyThe function now accepts
undefinedandnullvalues but uses a non-null assertion (!) which tells TypeScript to ignore the possibility of nullish values. While this works because of JavaScript's coercion (null/undefined become 0 in bitwise operations), it can be confusing and potentially risky.Consider explicitly handling nullish values for better clarity and type safety:
- export const matchBinaryMask = (value: number | undefined | null, mask: number) => { - return (value! & mask) === mask; - }; + export const matchBinaryMask = (value: number | undefined | null, mask: number) => { + // Explicitly handle nullish values - they behave like 0 in bitwise operations + if (value === null || value === undefined) { + return mask === 0; + } + return (value & mask) === mask; + };Or using nullish coalescing:
- export const matchBinaryMask = (value: number | undefined | null, mask: number) => { - return (value! & mask) === mask; - }; + export const matchBinaryMask = (value: number | undefined | null, mask: number) => { + return ((value ?? 0) & mask) === mask; + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/core/__tests__/internal/utils.test.ts(1 hunks)packages/core/__tests__/util/styleUtils.test.ts(3 hunks)packages/core/__tests__/view/mixins/CellMixin.test.ts(2 hunks)packages/core/src/editor/Editor.ts(2 hunks)packages/core/src/internal/utils.ts(1 hunks)packages/core/src/util/Constants.ts(1 hunks)packages/core/src/util/styleUtils.ts(3 hunks)packages/core/src/view/canvas/SvgCanvas2D.ts(5 hunks)packages/core/src/view/geometry/node/TextShape.ts(3 hunks)packages/core/src/view/plugins/CellEditorHandler.ts(4 hunks)packages/html/stories/EdgeCurvedAndRounded.stories.ts(1 hunks)packages/html/stories/HoverStyle.stories.js(1 hunks)packages/html/stories/Merge.stories.ts(1 hunks)packages/html/stories/SecondLabel.stories.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/html/stories/HoverStyle.stories.js
- packages/html/stories/Merge.stories.ts
- packages/core/tests/view/mixins/CellMixin.test.ts
- packages/html/stories/SecondLabel.stories.js
- packages/core/tests/internal/utils.test.ts
- packages/core/src/view/canvas/SvgCanvas2D.ts
- packages/core/src/util/styleUtils.ts
- packages/html/stories/EdgeCurvedAndRounded.stories.ts
- packages/core/src/editor/Editor.ts
- packages/core/tests/util/styleUtils.test.ts
- packages/core/src/util/Constants.ts
- packages/core/src/view/plugins/CellEditorHandler.ts
- packages/core/src/view/geometry/node/TextShape.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build
- GitHub Check: build (windows-2022)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (macos-14)
BREAKING CHANGES: The
constants.FONTenum has been removed and replaced by theconstants.FONT_STYLE_FLAGvalue object.Notes
Covers #378
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores