fix: adjust useWatermark logic
#4896
Conversation
|
WalkthroughThe changes in this pull request focus on enhancing the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/effects/hooks/src/use-watermark.tsOops! Something went wrong! :( ESLint: 9.14.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs 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 (
|
![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
🧹 Outside diff range and nitpick comments (3)
playground/src/views/demos/features/watermark/index.vue (2)
29-29: Consider caching timestamp updatesAdding a timestamp to the watermark content will cause the watermark to update frequently if
createWaterMarkis called multiple times. Consider caching the timestamp or updating it at longer intervals if frequent updates aren't necessary.- content: `hello my watermark\n${new Date().toLocaleString()}`, + content: `hello my watermark\n${new Date().toLocaleString('en-US', { hour12: false })}`,
65-83: Consider UX improvements and internationalizationThe button states correctly reflect the watermark lifecycle, but there are some suggestions for improvement:
- The distinction between "create" and "update" buttons might be confusing to users. Consider combining them into a single "Apply Watermark" button.
- The button text is in Chinese and should be internationalized for better maintainability.
<Button :disabled="!!watermark" class="mr-2" type="primary" - @click="recreateWaterMark" + @click="createWaterMark" > - 创建水印 + {{ t('features.watermark.create') }} </Button> - <Button - :disabled="!watermark" - class="mr-2" - type="primary" - @click="createWaterMark" - > - 更新水印 - </Button> <Button :disabled="!watermark" danger @click="destroyWatermark"> - 移除水印 + {{ t('features.watermark.remove') }} </Button>packages/effects/hooks/src/use-watermark.ts (1)
75-81: Consider translating the Chinese comment to EnglishThe implementation effectively prevents multiple hook registrations during route changes. However, for better maintainability and consistency, consider translating the Chinese comment to English.
- // 只在第一次调用时注册卸载钩子，防止重复注册以致于在路由切换时销毁了水印 + // Register unmount hook only on first call to prevent multiple registrations that would destroy watermark on route changes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/effects/hooks/src/use-watermark.ts(3 hunks)playground/src/views/demos/features/watermark/index.vue(3 hunks)
🔇 Additional comments (5)
playground/src/views/demos/features/watermark/index.vue (2)
7-7: LGTM: Improved state management with watermark
The addition of the watermark state from useWatermark enables proper button state management and synchronization.
9-12: Clarify the purpose of recreateWaterMark with empty settings
The function destroys the watermark and recreates it with an empty configuration object. This might lead to unexpected behavior as no default settings are specified.
Consider either:
- Passing default settings instead of an empty object
- Reusing the last known configuration
async function recreateWaterMark() {
destroyWatermark();
- await updateWatermark({});
+ await updateWatermark({
+ // Add default settings here
+ content: 'Default Watermark',
+ globalAlpha: 0.5,
+ });
}packages/effects/hooks/src/use-watermark.ts (3)
3-8: LGTM! Well-structured state management additions
The new imports and state management additions effectively support the PR objectives. The unmountedHooked ref provides a clean solution to prevent multiple hook registrations.
68-72: LGTM! Robust cleanup implementation
The destruction logic is well-structured with proper null checks and state cleanup. The preference update is correctly placed after the watermark cleanup.
86-86: LGTM! Improved encapsulation with readonly
Making the watermark ref readonly prevents uncontrolled external modifications, ensuring that all watermark lifecycle changes go through the provided methods.
Description
修复以下问题：
useWatermark的页面，无论是否实际创建或修改过水印，在切换到其它路由时都会自动销毁全局水印。这是不合理的useWatermark操作全局水印时没能同步更新preferences中的水印设置Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Bug Fixes
Refactor