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.

Sign up for GitHub

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

Jump to bottom

fix(components): [notification] type declaration error and four types of methods are missing context parameters #18951

Merged
merged 3 commits into from
Nov 29, 2024
Merged

fix(components): [notification] type declaration error and four types of methods are missing context parameters #18951

merged 3 commits into from
Nov 29, 2024

Conversation

YiMo1
Copy link
Contributor

@YiMo1 Nov 20, 2024 •
edited
Loading

fix #18941

相关PR：#6367、#6368
不知道是不是这个功能的作者忘记了这个声明以及四种类型方法的上下文参数，但在 message 组件中并未缺少这些东西。

Related PR: #6367、#6368
I don't know if the author of this feature forgot this declaration and the context parameters of the four types of methods, but these things are not missing in the message component.

@pull-request-triage Pull Request Triage
Copy link

👋 @YiMo1, thank you for contributing element-plus.

  • You can comment with /label Components:[component_name] to add a label for which component you are working on.
  • You may join our Discord for staying tuned.

@github-actions GitHub Actions
Copy link

Hello @YiMo1, thank you for contributing to element-plus, please see our guideline to see how to make contribution

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 20, 2024 •
edited
Loading

@github-actions github-actions bot added the CommitMessage::Qualified Qualified commit message label Nov 20, 2024
@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Nov 20, 2024 •
edited
Loading

Open in Stackblitz

pnpm add https://pkg.pr.new/element-plus/element-plus@18951

commit: 5eec5a9

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 20, 2024 •
edited
Loading

🧪 Playground Preview: https://element-plus.run/?pr=18951
Please comment the example via this playground if needed.

Copy link
Contributor Author

Nov 20, 2024 •
edited
Loading

element-plus/packages/components/notification/src/notify.ts

Line 38 in b80744c

function (options = {}, context: AppContext | null = null) {

element-plus/packages/components/notification/src/notify.ts

Line 87 in b80744c

vm.appContext = context ?? notify._context

这两行代码看样子像是冲突的，如果 context 允许为 null 的话，那么它应该是有用的，比如不需要上下文的时候。既然是有用的，那么就不应该在 null 的时候继承全局上下文。我认为可以做出以下更改，你们觉得呢？

These two lines of code seem to conflict. If context allows null, then it should be useful, such as when context is not needed. Since it is useful, it should not inherit the global context when it is null. I think the following changes can be made, What do you think?

function (options = {}, context?: AppContext | null)

vm.appContext = isUndefined(context) ? notify._context : context

@DDDDD12138 @btea

@DDDDD12138
Copy link
Contributor

我觉得 null 和 undefined 应该都代表没有吧？没有找到那里有确切用到 null ，所以需要再引入一个 isUndefined 来做判断嘛？🤔

抱歉，我在这个仓库可能没有权限，仅供参考，等其他佬来看看吧

Copy link
Contributor Author

我觉得 null 和 undefined 应该都代表没有吧？没有找到那里有确切用到 null ，所以需要再引入一个 isUndefined 来做判断嘛？🤔

如果是这样的话，完全没必要在形参上声明允许为 null，直接标注为 AppContext 就可以了，既然声明了可以为 null 那应该是有作用才会刻意去标注。

If that's the case, there's no need to declare null on the parameter, just annotate it as AppContext. Since it's declared that it can be null, it should be useful to intentionally annotate it.

@tolking tolking merged commit 8363b72 into element-plus:dev Nov 29, 2024
12 checks passed
@element-bot element-bot mentioned this pull request Nov 29, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@btea btea btea approved these changes

@tolking tolking tolking approved these changes

Assignees

YiMo1

Labels
CommitMessage::Qualified Qualified commit message Contribution::Community Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TypeScript] [notification] el-notification use app context inheritance cause ts(2554)
4 participants
@DDDDD12138 @tolking @btea