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

[Chat] Added Image Loading Placeholder to Chat Messages #3861

Merged
merged 8 commits into from
Dec 7, 2023

Conversation

jpeng-ms
Copy link
Member

@jpeng-ms jpeng-ms commented Dec 5, 2023

What

Adding image placeholder for messages with inline images

Why

to improve UX

How Tested

check the storybook page:

  • test it via CallWithChat, OR
  • test it via Message Thread Component page

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.

multiple image test

image

Dark Light Mode Testing:

dark-light-mode.mov

Success flow

sucess-flow.mov

Error flow

error-testing.mov

Copy link
Contributor

github-actions bot commented Dec 5, 2023

Copy link
Contributor

github-actions bot commented Dec 5, 2023

Chat bundle size is increased❗.

  • Current size: 1526088
  • Base size: 1525706
  • Diff size: 382

Copy link
Contributor

github-actions bot commented Dec 5, 2023

CallWithChat bundle size is increased❗.

  • Current size: 6399062
  • Base size: 6398680
  • Diff size: 382

Copy link
Contributor

github-actions bot commented Dec 5, 2023

Calling bundle size is increased❗.

  • Current size: 5586057
  • Base size: 5586056
  • Diff size: 1

Copy link
Contributor

github-actions bot commented Dec 5, 2023

@azure/communication-react jest test coverage for stable.

Lines Statements Functions Branches
Base 21188 / 33140
63.93%
21188 / 33140
63.93%
578 / 1001
57.74%
1686 / 2673
63.07%
Current 21205 / 33157
63.95%
21205 / 33157
63.95%
578 / 1001
57.74%
1685 / 2672
63.06%
Diff 17 / 17
0.02%
17 / 17
0.02%
0 / 0
0%
-1 / -1
-0.01%

Copy link
Contributor

github-actions bot commented Dec 5, 2023

@azure/communication-react jest test coverage for beta.

Lines Statements Functions Branches
Base 43445 / 68602
63.32%
43445 / 68602
63.32%
903 / 1943
46.47%
2586 / 4160
62.16%
Current 43431 / 68619
63.29%
43431 / 68619
63.29%
903 / 1943
46.47%
2567 / 4147
61.9%
Diff -14 / 17
-0.03%
-14 / 17
-0.03%
0 / 0
0%
-19 / -13
-0.26%

Copy link
Contributor

github-actions bot commented Dec 5, 2023

Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot.

@jpeng-ms jpeng-ms added the update_snapshots Set this label to request automated update of UI snapshots label Dec 5, 2023
@github-actions github-actions bot removed the update_snapshots Set this label to request automated update of UI snapshots label Dec 5, 2023
@jpeng-ms jpeng-ms changed the title [Chat] [Chat] Added Image Loading Placeholder to Chat Messages Dec 5, 2023
Copy link
Contributor

github-actions bot commented Dec 5, 2023

Copy link
Contributor

github-actions bot commented Dec 6, 2023

@jpeng-ms jpeng-ms marked this pull request as ready for review December 6, 2023 00:36
@jpeng-ms jpeng-ms requested review from a team as code owners December 6, 2023 00:36
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it has changed the default scrolling behaviour?

Copy link
Member Author

Choose a reason for hiding this comment

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

i actually think the "after" is correct since it scrolls to the bottom? also i feel like there's might be a race condition when snapshot is taken (maybe rendering the placeholder caused a few ms delay so snapshot is now taken when thread is scrolled to button)

Copy link
Contributor

github-actions bot commented Dec 6, 2023

Copy link
Contributor

github-actions bot commented Dec 6, 2023

Copy link
Member

@Leah-Xia-Microsoft Leah-Xia-Microsoft left a comment

Choose a reason for hiding this comment

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

Looks nice.

Copy link
Contributor

github-actions bot commented Dec 7, 2023

@jpeng-ms jpeng-ms merged commit 9af4768 into main Dec 7, 2023
37 checks passed
@jpeng-ms jpeng-ms deleted the john/add-image-loading-placeholder branch December 7, 2023 18: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.

8 participants