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.

Sign up for GitHub

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

Jump to bottom

Added new marker Badge for Report unread messages #4603

Merged
merged 9 commits into from
Aug 13, 2021
Merged

Added new marker Badge for Report unread messages #4603

merged 9 commits into from
Aug 13, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Aug 12, 2021 •
edited
Loading

Details

#4475 (comment)

Fixed Issues

$ Fixes #4475

Tests | QA Steps

  1. Open any chat on the newest WEB.
  2. Scroll back in the history of messages At 6-8 messages.
  3. Send a message from another device to active Chat.
  4. You should see a new message badge.
  5. Scroll back to the Bottom and the Badge should Hide.
  6. Follow steps from 2-4, Now click on the New messages over the badge, you should reach the bottom of the chat & the badge should hide.
  7. Follow steps from 2-4, Now click on the x over the badge, badge should hide.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web | desktop

image

Mobile Web

marker-m

iOS

image

Android

marker-a

@parasharrajat parasharrajat requested a review from a team as a code owner August 12, 2021 12:21
@MelvinBot MelvinBot requested review from Beamanator and removed request for a team August 12, 2021 12:21
@Beamanator
Copy link
Contributor

@parasharrajat please ping me when you're done & ready for a review

@parasharrajat
Copy link
Member Author

@Beamanator you can start the review meanwhile I will update the screens.

@parasharrajat
Copy link
Member Author

@Beamanator Ready to roll.

@Beamanator
Copy link
Contributor

@shawnborton I'm requesting you double-check the marker design before merging this (I'm still testing btw) - Also if there's only 1 new message I think the marker should say "1 new message" instead of "1 new messages" (plural) - please let me know if you agree!

@shawnborton
Copy link
Contributor

Looks pretty great to me, nice work Rajat!

@parasharrajat
Copy link
Member Author

parasharrajat commented Aug 13, 2021 •
edited
Loading

@Beamanator let me know the plural thing if that is something we want.

@shawnborton
Copy link
Contributor

Oh, forgot to mention that I agree with @Beamanator about the plural phrasing.

Beamanator
Beamanator requested changes Aug 13, 2021
View reviewed changes
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Lots of little comments but overall it works really well and looks great!

Here's a screenshot of testing in iOS, feel free to add it to your OP 👍

Screen Shot 2021-08-13 at 12 33 27 PM

src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionsView.js Show resolved Hide resolved
src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved
src/pages/home/report/MarkerBadge.js Outdated Show resolved Hide resolved
src/pages/home/report/MarkerBadge.js Outdated Show resolved Hide resolved
src/styles/styles.js Outdated Show resolved Hide resolved
src/styles/styles.js Show resolved Hide resolved
src/languages/en.js Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member Author

@Beamanator Updated.

@parasharrajat
Copy link
Member Author

parasharrajat commented Aug 13, 2021 •
edited
Loading

@Beamanator all 👍 ?

Beamanator
Beamanator reviewed Aug 13, 2021
View reviewed changes
src/languages/es.js Outdated
@@ -135,6 +135,9 @@ export default {
reportActionsView: {
beFirstPersonToComment: 'Sé el primero en comentar',
},
reportActionsViewMarkerBadge: {
newMsg: ({count}) => `${count} nuevo${count > 1 ? 's' : ''} mensaje${count > 1 ? 's' : ''}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe nuevo should come after mensaje

Copy link
Member Author

Choose a reason for hiding this comment

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

Please tell me what is correct for new messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

"# new messages" => # mensajes nuevos
"# new message" => # mensaje nuevo

@parasharrajat
Copy link
Member Author

@Beamanator Thanks for the tip. Updated. Please let me know if something else is missing.

@Beamanator Beamanator self-requested a review August 13, 2021 15:05
Beamanator
Beamanator approved these changes Aug 13, 2021
View reviewed changes
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Looks great! Once E2E tests finish and pass I'll merge 👍

@Beamanator Beamanator merged commit 9368288 into Expensify:main Aug 13, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify OSBotify mentioned this pull request Aug 13, 2021
Closed
53 tasks
@parasharrajat parasharrajat mentioned this pull request Aug 16, 2021
Closed
@OSBotify OSBotify mentioned this pull request Aug 17, 2021
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Beamanator in version: 1.0.85-10 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@kavimuru kavimuru mentioned this pull request Aug 17, 2021
Closed
@isagoico isagoico mentioned this pull request Aug 17, 2021
Closed
@kavimuru kavimuru mentioned this pull request Aug 19, 2021
Closed
@botify
Copy link

botify commented Aug 25, 2021

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-01. 🎊

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.0.86-11 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@botify
Copy link

botify commented Aug 30, 2021

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-06. 🎊

@eVoloshchak
Copy link
Contributor

Dropping a note this caused a regression: #11584
(only if Mark as unread functionality existed at the time)

@eVoloshchak eVoloshchak mentioned this pull request Nov 10, 2022
Closed
@parasharrajat
Copy link
Member Author

I won't call it a regression. I you will go over the issue details and proposals, it says that clicking the badge will take you to the latest message l. It was implemented this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Beamanator Beamanator Beamanator approved these changes

Assignees

@parasharrajat parasharrajat

Labels
Design
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

There's no notification of newly received message(s) if you are reading historical messages in the back scroll
6 participants
@parasharrajat @Beamanator @shawnborton @OSBotify @botify @eVoloshchak