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

fix: Update FollowingStep component to use ref for currentGuidePositions #1489

Merged
merged 21 commits into from
Mar 24, 2025

Conversation

neverbiasu
Copy link

No description provided.

Copy link

qiniu-prow bot commented Mar 20, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Author

@neverbiasu neverbiasu left a comment

Choose a reason for hiding this comment

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

@nighca 老师我这里可以Review了,表单提交已经测试成功。

@@ -84,9 +86,19 @@ function setFilterControls() {
filter.setFilter('backdrop', props.step.isBackdropControl, props.step.backdrops)
}

const isStepProcessing = ref(false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个没看到用的地方?


return Object.keys(props.step.taggingHandler).map((path) => {
const type = props.step.taggingHandler[path]
const { getElement } = useTag()
Copy link
Collaborator

Choose a reason for hiding this comment

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

useTag() 尽量拿出去,放在 setup script 下顶层做

findAndSetupElement()
}, 500)
},
{ immediate: true, deep: true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里应该不需要 deep: true;如果需要,大概率是什么地方实现得有问题


setFilterControls()

// Wait for modal to be shown
Copy link
Collaborator

Choose a reason for hiding this comment

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

#1489 (comment) 这里提到的问题一样,这里不应该需要去搞个 timeout 去特地等某个组件“渲染完成”

另外这里很奇怪的是,为什么给 stepType.value 赋值这件事情也要推迟到 modal 渲染完成后呢?

Copy link
Author

Choose a reason for hiding this comment

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

其实现在的场景会需要这个等待组件渲染,因为modal渲染的有点慢,如果不加的话Mask计算的高亮就会小一圈

Copy link
Collaborator

Choose a reason for hiding this comment

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

因为modal渲染的有点慢,如果不加的话Mask计算的高亮就会小一圈

这个是 Mask 的 bug,应该由 Mask 去解决,Mask 应该确保目标元素发生了动态的尺寸变化后还能正确地高亮之

Copy link
Collaborator

Choose a reason for hiding this comment

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

#1388 你可以看下这个 PR 以及相关的讨论,能正确地随着目标元素的动态变更去调整自己是 Mask 本来就预期做到的

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
Collaborator

Choose a reason for hiding this comment

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

OK,这里最好是把这个已知问题注释记一下,等 Mask 修复好了这里再把注释干掉

@@ -86,19 +115,18 @@ function setFilterControls() {
filter.setFilter('backdrop', props.step.isBackdropControl, props.step.backdrops)
}

const isStepProcessing = ref(false)
const isProcessing = ref(false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

我还是没看懂,这个 isProcessing 是干嘛的

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
Collaborator

Choose a reason for hiding this comment

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

为什么会出现“重复执行”呢

Copy link
Author

Choose a reason for hiding this comment

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

确实不会诶,那我删了

@qiniu-ci
Copy link

This PR has been deployed to the preview environment. You can explore it using the preview URL.

Warning

Please note that deployments in the preview environment are temporary and will be automatically cleaned up after a certain period. Make sure to explore it before it is removed. For any questions, contact the Go+ Builder team.

Copy link
Collaborator

@nighca nighca left a comment

Choose a reason for hiding this comment

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

上边提到的问题相对小,后续再处理吧;这个 PR 先 merge

stepType.value = step.type

await nextTick()
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个 await nextTick() 也很迷啊,在它之后看上去没有别的行为了,那它的存在是为了推迟什么呢?

@nighca nighca merged commit b25d713 into goplus:trailhead_guidance Mar 24, 2025
5 checks passed
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.

3 participants