Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add previous and next toggle to toolbar #320

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

madocto
Copy link

@madocto madocto commented Mar 19, 2024

Copy link

vercel bot commented Mar 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
image ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 15, 2024 7:50am

src/Preview.tsx Outdated
Comment on lines 28 to 29
onSwitchLeft?: () => void;
onSwitchRight?: () => void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onSwitchPrev
onSwitchNext

这样好点

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTP 里叫 onActive,感觉可以复用这个名字?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好,保持统一

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

前一个、后一个跟 active 语义上应该是没有关系的,我觉得 onSwitchPrev、onSwitchNext 好点

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.78%. Comparing base (4fe15d0) to head (b6c0b89).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #320   +/-   ##
=======================================
  Coverage   99.78%   99.78%           
=======================================
  Files          15       15           
  Lines         457      469   +12     
  Branches      128      134    +6     
=======================================
+ Hits          456      468   +12     
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@madocto madocto changed the title feat: add left and right switches to toolbar feat: add onSwitchPrev and onSwitchNext to toolbar Mar 19, 2024
@madocto madocto requested a review from afc163 March 19, 2024 13:27
@madocto madocto changed the title feat: add onSwitchPrev and onSwitchNext to toolbar feat: add onSwitchPrev and onSwitchNext to toolbar Mar 19, 2024
@madocto madocto changed the title feat: add onSwitchPrev and onSwitchNext to toolbar feat: add onActivePrev and onActiveNext to toolbar Mar 29, 2024
@madocto madocto requested a review from zombieJ March 29, 2024 09:42
README.md Outdated
@@ -180,6 +182,8 @@ type TransformAction =
zoomInIcon: React.ReactNode;
};
actions: {
onActivePrev?: () => void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type OnActive = (offset: number) -> void;

不需要前后,一个就够了。提供 offset 作为偏移量。这样如果是未来一次性翻10个也是同一个 API

@afc163
Copy link
Member

afc163 commented May 14, 2024

  1. 标题不太对。
  2. 测试覆盖率掉了。

@madocto madocto changed the title feat: add onActivePrev and onActiveNext to toolbar feat: add previous and next toggle to toolbar May 15, 2024
src/common.ts Outdated
@@ -11,3 +11,7 @@ export const COMMON_PROPS: (keyof Omit<ImageElementProps, 'src'>)[] = [
'useMap',
'alt',
];

export const DEFAULT_PREV_OFFSET = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

没必要抽常量,徒增 bundle size 哈~~

src/Preview.tsx Outdated
setEnableTransition(false);
resetTransform('next');
onChange?.(current + 1, current);
if (!Number.isInteger(offset) || offset === 0 || position < 0 || position > count - 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个时序看起来有问题,上面已经拿 offset 做计算了,下面再判断 offset 没什么必要。只要看 position 是否是合法的在范围内即可


const toolsNodeMap = new Map<ToolType, React.ReactNode>()

const toolsNode = tools.map(({ icon, onClick, type, disabled }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不需要遍历生成,直接搞个 renderOperation 方法就行了。以前的 tools 搞个数组看起来也没什么必要。

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里没太明白,看看是这个意思不

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants