Skip to content

Fix parameter passed to OC.Notification#1989

Merged
nickvergessen merged 2 commits intomasterfrom
fix-parameter-passed-to-oc-notification
Jul 15, 2019
Merged

Fix parameter passed to OC.Notification#1989
nickvergessen merged 2 commits intomasterfrom
fix-parameter-passed-to-oc-notification

Conversation

@danxuliu
Copy link
Member

Requires nextcloud/server#16369

OC.Notification.showTemporary expects a string as its first parameter (even if it contains HTML markup), but a jQuery object was passed instead; until now this just happened to work due to the internal implementation of that method, but this no longer works since the switch to Toastify in Nextcloud 17.

How to test

  • Use Chromium < 72 without the screensharing extension
  • Start a call
  • Share the screen

Result with this pull request

The notification shows Screensharing extension is required to share your screen.; clicking on it opens the extension page.

Result without this pull request

The notification shows [Object object].

danxuliu added 2 commits July 15, 2019 09:20
"OC.Notification.showTemporary" expects a string as its first parameter
(even if it contains HTML markup), but a jQuery object was passed
instead; until now this just happened to work due to the internal
implementation of that method, but this no longer works since the switch
to Toastify in Nextcloud 17.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@nickvergessen nickvergessen force-pushed the fix-parameter-passed-to-oc-notification branch from af24a4c to 6e9a8ca Compare July 15, 2019 07:20
@nickvergessen
Copy link
Member

Should we wait for the server PR with the merge?

@MorrisJobke
Copy link
Member

Requires nextcloud/server#16369

merged now

@nickvergessen nickvergessen merged commit 3938ed8 into master Jul 15, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix-parameter-passed-to-oc-notification branch July 15, 2019 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants