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(components): [el-message] config-provider message max attr #5063

Merged
merged 9 commits into from Jan 8, 2022

Conversation

btea
Copy link
Collaborator

@btea btea commented Dec 30, 2021

Please make sure these boxes are checked before submitting your PR, thank you!

  • Make sure you follow contributing guide English | (中文 | Español | Français).
  • Make sure you are merging your commits to dev branch.
  • Add some descriptions and refer to relative issues for your PR.

feat #4868

@github-actions
Copy link

github-actions bot commented Dec 30, 2021

@btea btea requested a review from emojiiii December 30, 2021 13:41
@btea btea changed the title Feat message add feat(components): [el-message] config-provider message max attr Dec 30, 2021
Copy link
Member

@jw-foss jw-foss left a comment

Choose a reason for hiding this comment

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

Left a couple comments for you to check, thanks.

@emojiiii
Copy link
Contributor

emojiiii commented Jan 4, 2022

I think we should discuss how to deal(discard/queue) with redundant messages if the maximum is exceeded @btea @JeremyWuuuuu

@btea
Copy link
Collaborator Author

btea commented Jan 4, 2022

IMO, redundant message should be discarded. If multiple messages are placed in the queue and pop up one by one, the user may switch to other pages, but the code logic is still popping up messages one after another, which will cause confusion.

@emojiiii
Copy link
Contributor

emojiiii commented Jan 4, 2022

IMO, redundant message should be discarded. If multiple messages are placed in the queue and pop up one by one, the user may switch to other pages, but the code logic is still popping up messages one after another, which will cause confusion.

But if they are discarded, some important information may be lost?

@btea
Copy link
Collaborator Author

btea commented Jan 4, 2022

IMO, redundant message should be discarded. If multiple messages are placed in the queue and pop up one by one, the user may switch to other pages, but the code logic is still popping up messages one after another, which will cause confusion.

But if they are discarded, some important information may be lost?

I think it depends on the user. If the user thinks the information is important, he can not set the max, or he can set the max a little larger.

@btea btea requested a review from jw-foss January 5, 2022 10:39
@emojiiii
Copy link
Contributor

emojiiii commented Jan 7, 2022

can you test when you use two config-provide and different max ? @btea

@btea
Copy link
Collaborator Author

btea commented Jan 7, 2022

Generally, only one config-provider will be used. If there are more than one, the latter will overwrite the former, and the other configurations are the same.

@emojiiii
Copy link
Contributor

emojiiii commented Jan 7, 2022

You're right. If you can add another unit test for this, it will be very robust

@btea
Copy link
Collaborator Author

btea commented Jan 7, 2022

Do you mean to add multiple config-provider and then set different max values?

@emojiiii
Copy link
Contributor

emojiiii commented Jan 7, 2022

Do you mean to add multiple config-provider and then set different max values?

yes

@btea
Copy link
Collaborator Author

btea commented Jan 7, 2022

Maybe it is better to modify the value of config directly like in the test code?

@emojiiii
Copy link
Contributor

emojiiii commented Jan 7, 2022

Maybe it is better to modify the value of config directly like in the test code?

I'm just worried about whether multiple config-provider can merge configuration items as expected

@btea
Copy link
Collaborator Author

btea commented Jan 7, 2022

Okay, I will add unit test about this.

@jw-foss
Copy link
Member

jw-foss commented Jan 8, 2022

Maybe it is better to modify the value of config directly like in the test code?

I'm just worried about whether multiple config-provider can merge configuration items as expected

IMHO, multiple provider is made by the developer, so that it is at their choice to make the call, so that we cannot handle this because they did what they did, that was completely on their own, so that we can just ignore it. Because the specialty about these global items (as designed this way) they should be able to accept that. Or maybe later we can try to pass the context to the call so that we can get the app context.

Copy link
Member

@jw-foss jw-foss left a comment

Choose a reason for hiding this comment

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

LGTM

@jw-foss jw-foss added this to the 1.3.0-beta.2 milestone Jan 8, 2022
@jw-foss jw-foss merged commit 70fa3e7 into element-plus:dev Jan 8, 2022
@element-bot element-bot mentioned this pull request Jan 8, 2022
3 tasks
@emojiiii emojiiii linked an issue Jan 21, 2022 that may be closed by this pull request
@btea btea deleted the feat-message-add branch May 30, 2022 02:48
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.

[Feature Request] el-message max attribute
3 participants