-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
[Notifier] [Mercure] Add options #54961
base: 7.2
Are you sure you want to change the base?
Conversation
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
this PR connect to this PR symfony/ux#1853 |
Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
I just noticed that |
you need this option in google chrome |
Might be the case, couldn't find any example/issue that this is the case, but why excluding other browsers and not use just a simple array to dynamically pass the possible options? My recommendation:
It also looks like the resulting activity stream doesn't fit the spec: https://www.w3.org/TR/activitystreams-vocabulary/#dfn-announce. This might fit the spec more: {
"@context":"https://www.w3.org/ns/activitystreams",
"type":"Announce",
"summary":"subject",
"mediaType": "application/json",
"content": "{\"body\":null,\"icon\":null,\"tag\":null,\"renotify\":false}"
} |
@chapterjason it's a good idea |
@ernie76 You don't need to create new ones, just modify it, that's what PRs are about, they are evolving. I really like the initial idea, as it gives more control without creating any JS. |
add "mediaType" => "application/json" to MercureOptions.php
body
, icon
, tag
,renotify
body
, icon
, tag
,renotify
@chapterjason is this so ok now? |
$this->topics = $topics ?? 'https://symfony.com/notifier'; | ||
|
||
parent::__construct($client, $dispatcher); | ||
} | ||
|
||
public function __toString(): string | ||
{ | ||
return sprintf('mercure://%s%s', $this->hubId, '?'.http_build_query(['topic' => $this->topics], '', '&')); | ||
return sprintf('mercure://%s%s', $this->hubId, null !== $this->topics ? '?'.http_build_query(['topic' => $this->topics], '', '&') : ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why checking for null if constructor always sets a topic in that case?
@@ -30,18 +30,17 @@ | |||
*/ | |||
final class MercureTransport extends AbstractTransport | |||
{ | |||
private HubInterface $hub; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reverted, we already had promoted properties
@@ -30,18 +30,16 @@ | |||
*/ | |||
final class MercureTransport extends AbstractTransport | |||
{ | |||
private string $hubId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be removed
public function __construct( | ||
private HubInterface $hub, | ||
private string $hubId, | ||
string|array|null $topics = null, | ||
?HttpClientInterface $client = null, | ||
?EventDispatcherInterface $dispatcher = null, | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please keep this
]); | ||
} | ||
|
||
public function testConstructWithParameters() | ||
{ | ||
$options = (new MercureOptions('/topic/1', true, 'id', 'type', 1)); | ||
$options = (new MercureOptions('/topic/1', true, 'id', 'type', 1, null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would provide a text here, rather than null
optionstest with options
src/Symfony/Component/Notifier/Bridge/Mercure/MercureTransport.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Notifier/Bridge/Mercure/MercureTransport.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Notifier/Bridge/Mercure/Tests/MercureOptionsTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Notifier/Bridge/Mercure/Tests/MercureOptionsTest.php
Outdated
Show resolved
Hide resolved
…ionsTest.php Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
…ionsTest.php Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
@@ -1,6 +1,9 @@ | |||
CHANGELOG | |||
========= | |||
|
|||
* Add content to MercureOptions.php for Symfony UX Notify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a new 7.2 section and add only
* Add `content` option
….php Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
….php Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
@@ -77,6 +77,8 @@ protected function doSend(MessageInterface $message): SentMessage | |||
'@context' => 'https://www.w3.org/ns/activitystreams', | |||
'type' => 'Announce', | |||
'summary' => $message->getSubject(), | |||
'mediaType' => 'application/json', | |||
'content' => $options->getContent(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it fit correctly it should be serialized to json here. (Only the value of the content entry)
For now I think it is enough to just inject the Serializer and always encode to json. To keep this PR simple.
I would also add an test to cover this case.
Hey @ernie76 a bit busy the last days, only one thing, everything else looks good. 👍🏼 |
Added body, icon, tag ,renotify for desktop notifications in Symfony UX
This pull-request are need changes in src/Notify/assets/dist/controller.js I have made a pull request there too.