Skip to content

Add onFetchProfile to teams adapter option#2680

Merged
PorterNan merged 17 commits intomainfrom
jinan/onFetchProfile-adapter
Feb 1, 2023
Merged

Add onFetchProfile to teams adapter option#2680
PorterNan merged 17 commits intomainfrom
jinan/onFetchProfile-adapter

Conversation

@PorterNan
Copy link
Copy Markdown
Contributor

@PorterNan PorterNan commented Jan 25, 2023

What

Add onFetchProfile to teams adapter option

  • Add a util function to memorize and modify participant ref and generate when something changes
  • Add a sync modifier which will modify the state and able to handle side effects
    Code sample:
    image
    Result:
    image

Why

How Tested

Process & policy checklist

  • I have updated the project documentation to reflect my changes if necessary.
  • I have read the CONTRIBUTING documentation.

Is this a breaking change?

  • This change causes current functionality to break.

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 26, 2023

Calling bundle size is increased❗.

  • Current size: 5560628
  • Base size: 5558914
  • Diff size: 1714

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 26, 2023

CallWithChat bundle size is increased❗.

  • Current size: 5944901
  • Base size: 5943415
  • Diff size: 1486

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 26, 2023

Chat bundle size is increased❗.

  • Current size: 5643542
  • Base size: 5643541
  • Diff size: 1

Comment thread packages/communication-react/review/beta/communication-react.api.md
/* @conditional-compile-remove(PSTN-calls) */
alternateCallerId,
/* @conditional-compile-remove(rooms) */
/* @conditional-compile-remove(teams-identity-support) */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should this be dependent on both CC statements here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, I was adding this for both adapter, but now it is only for teams one

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, interesting question, I looked at the code a bit, this dependency is like "this or that", so either code being available in stable then we should not remove this line anymore - current conditional compilation logic works for this part

Some other dependency is like "This and that", so we need both code to enable the line.

Comment thread packages/communication-react/review/beta/communication-react.api.md
@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 1, 2023

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 1, 2023

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 1, 2023

Comment thread samples/Calling/src/app/views/CallScreen.tsx Outdated
* The onFetchProfile is fetch-and-forget one time action for each user, once a user profile is updated, the value will be cached
* and would not be updated again within the lifecycle of adapter.
*/
onFetchProfile?: OnFetchProfileCallback;
Copy link
Copy Markdown
Member

@JamesBurnside JamesBurnside Feb 1, 2023

Choose a reason for hiding this comment

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

(not for this PR, just a thought)
Should we provide any way to know if the information was populated from the calling sdk already inside this callback?
E.g. I want to come along and only modify profiles were data doesn't already exist.

An alternative to this is in an example usage we pull out of the call remoteparticipants object by default and only do some fancy graph fetch if its undefined and a teams user

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, we can do (id: string, displayName?: string) => Profile for this case

Let's check this in for beta release first, then I will open a new PR to add this one

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 1, 2023

Copy link
Copy Markdown
Member

@JamesBurnside JamesBurnside left a comment

Choose a reason for hiding this comment

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

🚀🚀🚀

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 1, 2023

@PorterNan PorterNan merged commit 46c5aea into main Feb 1, 2023
@PorterNan PorterNan deleted the jinan/onFetchProfile-adapter branch February 1, 2023 23:16
PorterNan pushed a commit that referenced this pull request Feb 1, 2023
* Add onFetchProfile to teams adapter

---------

Co-authored-by: Nan Jiang <jinan@microsoft.com>
edwardlee-msft added a commit that referenced this pull request Feb 3, 2023

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Nan Jiang <jinan@microsoft.com>
Co-authored-by: edwardlee-msft <edwardlee@microsoft.com>
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.

5 participants