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

Fix gsd mode pinned room not on the top #6849

Merged
merged 10 commits into from
Jan 11, 2022
Merged

Conversation

K4tsuki
Copy link
Contributor

@K4tsuki K4tsuki commented Dec 21, 2021

Details

Fixed Issues

$ #6563

Tests

  1. On LHN there must be two or more pinned reports
  2. On LHN there must be one or more unpinned unread reports or have outstanding IOU
  3. The unread reports name should be between or before the pinned reports ordered alphabetically
  4. Change LHN mode to focus mode
  5. The pinned reports must be on top, alphabetically ordered
  6. The unpinned reports that have unread or outstanding IOU must be bellow pinned reports ordered alphabetically

QA Steps

  1. On LHN there should be two or more pinned reports
  2. On LHN there must be one or more unpinned unread reports or have outstanding IOU
  3. The unread report names should be between or before the pinned reports ordered alphabetically
  4. Change LHN mode to focus mode
  5. The pinned reports must be on top, ordered alphabetically
  6. The unpinned reports that have unread or outstanding IOU must be bellow pinned reports ordered alphabetically

Tested On

  • [v] Web
  • [v] Mobile Web
  • Desktop
  • iOS
  • [v] Android

Screenshots

Before

focus_before


After

Web

focus_web

Mobile Web

focus_mobile_web

Desktop

iOS

Android

focus_anroid_2

@github-actions
Copy link
Contributor

github-actions bot commented Dec 21, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@K4tsuki
Copy link
Contributor Author

K4tsuki commented Dec 21, 2021

I have read the CLA Document and I hereby sign the CLA

@K4tsuki K4tsuki marked this pull request as ready for review December 22, 2021 14:54
@K4tsuki K4tsuki requested a review from a team as a code owner December 22, 2021 14:54
@MelvinBot MelvinBot removed the request for review from a team December 22, 2021 14:54
tests/unit/OptionsListUtilsTest.js Outdated Show resolved Hide resolved
tests/unit/OptionsListUtilsTest.js Outdated Show resolved Hide resolved
tests/unit/OptionsListUtilsTest.js Show resolved Hide resolved
tests/unit/OptionsListUtilsTest.js Outdated Show resolved Hide resolved
src/libs/OptionsListUtils.js Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member

Also, it's strange but I can't see who signed the commits. Could you please resign your commits so that we can see your name with the GPG key?

tests/unit/OptionsListUtilsTest.js Outdated Show resolved Hide resolved
tests/unit/OptionsListUtilsTest.js Outdated Show resolved Hide resolved
@K4tsuki
Copy link
Contributor Author

K4tsuki commented Dec 23, 2021

Also, it's strange but I can't see who signed the commits. Could you please resign your commits so that we can see your name with the GPG key?

I have signed the last commit.

@parasharrajat
Copy link
Member

parasharrajat commented Dec 23, 2021

@K4tsuki You would need to sign all the commits and also please purge the last four unnecessary commits. You would need to rebase them. Thanks.

@K4tsuki
Copy link
Contributor Author

K4tsuki commented Dec 24, 2021

@K4tsuki You would need to sign all the commits and also please purge the last four unnecessary commits. You would need to rebase them. Thanks.

Done

parasharrajat
parasharrajat previously approved these changes Dec 27, 2021
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM.

cc: @Luke9389

@mallenexpensify
Copy link
Contributor

@Luke9389 is back on Monday and will review then, apologies for the delay @K4tsuki

@K4tsuki
Copy link
Contributor Author

K4tsuki commented Dec 29, 2021

@Luke9389 is back on Monday and will review then, apologies for the delay @K4tsuki

No problem @mallenexpensify .

Luke9389
Luke9389 previously approved these changes Jan 4, 2022
Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@K4tsuki K4tsuki dismissed stale reviews from Luke9389 and parasharrajat via b991b46 January 4, 2022 03:29
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

🎀 👀 🎀 C+ reviewed

@K4tsuki
Copy link
Contributor Author

K4tsuki commented Jan 10, 2022

@Luke9389 All cheks have passed.

@Luke9389 Luke9389 merged commit 2e5b6c3 into Expensify:main Jan 11, 2022
@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
Copy link
Contributor

🚀 Deployed to staging by @Luke9389 in version: 1.1.27-2 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Luke9389 in version: 1.1.27-3 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Luke9389 in version: 1.1.27-3 🚀

platform result
🤖 android 🤖 skipped 🚫
🖥 desktop 🖥 skipped 🚫
🍎 iOS 🍎 skipped 🚫
🕸 web 🕸 skipped 🚫

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Jag96 in version: 1.1.29-5 🚀

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

@K4tsuki
Copy link
Contributor Author

K4tsuki commented Jan 14, 2022

@parasharrajat How about sorting issue here?
Currently the sorting is case sensitive. Should we make it case insensitive?
Should we make another pull request?

@mallenexpensify
Copy link
Contributor

@parasharrajat can you reply to @K4tsuki above?
#6849 (comment)

@parasharrajat
Copy link
Member

Aha, that's a good point. But this is a question for slack. @K4tsuki

@K4tsuki
Copy link
Contributor Author

K4tsuki commented Jan 22, 2022

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