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

Add Notification component multiLine prop #6670

Merged
merged 8 commits into from
Oct 14, 2021

Conversation

WiXSL
Copy link
Contributor

@WiXSL WiXSL commented Oct 13, 2021

Follow up of #6191

Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Thanks!

packages/ra-core/src/sideEffect/useNotify.spec.tsx Outdated Show resolved Hide resolved
@djhi djhi added this to the 3.19 milestone Oct 13, 2021
@WiXSL
Copy link
Contributor Author

WiXSL commented Oct 13, 2021

I have divided #6191 into two PRs, #6670 and #6671

Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

What about @fzaninotto comment on the previous PR ?

We must switch to a simpler signature: notify(message, options). This can be done in a backwards-compatible way by inspecting the type of the second argument at runtime. TypeScript may get in the way, but it supports multiple call signatures.

edit Just saw the other pr

@WiXSL WiXSL requested a review from djhi October 13, 2021 19:21
Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Just noticed that it seems you forgot to actually apply the styles required for multiline

@WiXSL WiXSL requested a review from djhi October 14, 2021 11:22
@WiXSL WiXSL requested a review from djhi October 14, 2021 12:51
Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Thanks!

@djhi djhi merged commit 90899d0 into marmelab:master Oct 14, 2021
@WiXSL WiXSL deleted the add-multiline-notifications branch October 14, 2021 13:03
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

2 participants