Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Notifier] Add options to Microsoft Teams notifier#40738

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@OskarStark
Copy link
Contributor

@OskarStarkOskarStark commentedApr 8, 2021
edited
Loading

QA
Branch?5.4
Bug fix?no
New feature?yes
Deprecations?no
TicketsFollows#39007
LicenseMIT
Doc PRsymfony/symfony-docs#15288

After rework:

CleanShot 2021-04-15 at 09 40 45@2x

@jderussejderusse added this to the5.x milestoneApr 10, 2021
@OskarStarkOskarStarkforce-pushed thenotifier/microsot-teams-options branch from14eea2a to920961eCompareApril 12, 2021 18:43
Copy link
Member

@NyholmNyholm left a comment

Choose a reason for hiding this comment

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

Thank you. This looks way better now.

I've not tested the code, but I trust that you have.

@OskarStarkOskarStarkforce-pushed thenotifier/microsot-teams-options branch 2 times, most recently fromd89f4b5 to767698eCompareApril 14, 2021 08:26
Copy link
Member

@chalasrchalasr left a comment
edited
Loading

Choose a reason for hiding this comment

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

It seems that some interfaces are not actually needed.
Interfaces make maintenance harder so it must be worth it. I strongly think those should be removed, we can reconsider later if a use case comes out.

@OskarStarkOskarStarkforce-pushed thenotifier/microsot-teams-options branch fromd997961 to3fc7247CompareMay 20, 2021 11:01
@OskarStarkOskarStark requested a review fromchalasrMay 20, 2021 11:02
@OskarStarkOskarStarkforce-pushed thenotifier/microsot-teams-options branch 2 times, most recently from57c793d to0039b9cCompareMay 28, 2021 14:00
@OskarStarkOskarStarkforce-pushed thenotifier/microsot-teams-options branch 2 times, most recently from4d275c8 tod86f7b4CompareJune 9, 2021 19:00
@OskarStarkOskarStark requested a review fromderrabusJune 9, 2021 19:01
Copy link
Member

@jderussejderusse left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of the protectedoptions and everybody writing into it.
I wonder if the property shouldn't be private, and each class is responsible for its own properties. Then change thetoArray method for something like

publicfunctiontoArray():array{returnparent::toArray() +$this->options + ['@action' =>'FooBar'];}

@OskarStark
Copy link
ContributorAuthor

I'm not a big fan of the protected options and everybody writing into it.
I wonder if the property shouldn't be private, and each class is responsible for its own properties. Then change the toArray method for something like

@jderusse makes sense, did that 👍

@OskarStarkOskarStarkforce-pushed thenotifier/microsot-teams-options branch from96d3019 toe33faaeCompareJune 18, 2021 06:24
@fabpotfabpotforce-pushed thenotifier/microsot-teams-options branch frome33faae tod039ce7CompareJune 23, 2021 14:22
@fabpot
Copy link
Member

Thank you@OskarStark.

OskarStark reacted with rocket emoji

@fabpotfabpot merged commitfa120c0 intosymfony:5.4Jun 23, 2021
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestJul 27, 2021
…milKubicki)This PR was squashed before being merged into the 5.4 branch.Discussion----------[Notifier] Documentation for Microsoft Teams OptionsDocs forsymfony/symfony#40738Replaces#15232Commits-------3c98ba8 [Notifier] Documentation for Microsoft Teams Options
This was referencedNov 5, 2021
@OskarStarkOskarStark deleted the notifier/microsot-teams-options branchJune 23, 2025 08:37
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@NyholmNyholmNyholm approved these changes

@fabpotfabpotfabpot approved these changes

@chalasrchalasrAwaiting requested review from chalasr

@derrabusderrabusAwaiting requested review from derrabus

@jderussejderusseAwaiting requested review from jderusse

+1 more reviewer

@KamilKubickiKamilKubickiKamilKubicki requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@OskarStarkOskarStark

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

8 participants

@OskarStark@Nyholm@KamilKubicki@fabpot@jderusse@chalasr@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp