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

Add Unread Action Indicator #1570

Merged
merged 7 commits into from
Mar 2, 2021

Conversation

lucas-neuhaus-dev
Copy link
Contributor

@lucas-neuhaus-dev lucas-neuhaus-dev commented Feb 26, 2021

@marcaaron @roryabraham

Details

Created a "last unread" indicator on the Reports View. It appears on the top of the last unread action and fades out after a set time.
It doesn't show up when the report is active and receives a new message.

About scrollToIndex:
As mentioned on the issue page, I couldn't find a trivial solution to this, since scrollToIndex isn't precise at all with items with dynamic height. I still want to try to calculate the height and use it with scrollToOffset(), but I thought it would be better to create a PR with the rest of the code first.

@shawnborton
Animation info:

  • Timeout time: 3000 ms;
  • Duration: 500 ms;
  • Easing: Easing.ease;

Just for reference if you want me to try some other values out.

Notes:

  1. I actually did not want to use position: absolute, and it was working fine everywhere just using height: 0. But it seems like ios <Text /> doesn't like it very much, since it was not showing up (the line was showing, the text wasn't). It was working everywhere else;
  2. There is a behavior that I really dislike right now. On small screens, when the Sidebar is open, and a new message is received on the active report, it is set as read, and the indicator doesn't show up (if the user re-enters the report). I actually wrote some code around it with withWindowDimensions, and adding the IS_SIDEBAR_SHOWN onyx prop, but decided to remove it. I believe the best solution would be to start changing the code deeper down, to make sure an action is not set as read if isSmallScreen and IS_SIDEBAR_SHOWN === true. It would still need some refactoring on the ReportActionsView code, but I believe it is the best approach;
  3. Maybe anticipating a question here: The renderItem() component is wrapped inside a <View /> instead of a <React.Fragment /> for compatibility between the native and web/desktop <InvertedFlatList />. If we don't do it, the indicator actually shows up on the bottom of the message instead of on the top when on native.

Fixed Issues

Fixes #1498

Tests

  1. Make sure the user/group that is sending a new message is not active;
  2. Receive a new message;
  3. Open the report, it should show the indicator on the top of the last unread message;
  4. Verify the indicator fades out after a few seconds;
  5. If another message is sent while the report is active, verify the indicator doesn't show up again.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Observation: After I got all the images, I realized the NEW text was not bold as on the reference, as well as the spacing was maybe too high.
Because it takes a while to take new images, and it is very simple, I just changed the Mobile-Web image after making the changes. I hope it is ok.
So, please, the text IS BOLD. Sorry about that.

Web

web

Mobile Web

mobile web

Desktop

deskotp

iOS

ios

Android

android

@lucas-neuhaus-dev lucas-neuhaus-dev requested a review from a team as a code owner February 26, 2021 15:31
@botify botify requested review from bondydaa and removed request for a team February 26, 2021 15:31
src/styles/styles.js Outdated Show resolved Hide resolved
@lucas-neuhaus-dev
Copy link
Contributor Author

@shawnborton
New results:
gif
image

@shawnborton
Copy link
Contributor

Looks great, thanks for the quick work!

shawnborton
shawnborton previously approved these changes Feb 26, 2021
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

This LGTM but will leave the real review for @bondydaa

src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved
@lucas-neuhaus-dev
Copy link
Contributor Author

lucas-neuhaus-dev commented Feb 26, 2021

@roryabraham
I'm having some unexpected problems when trying to receive just the active report from the store, as you suggested.
I pushed a commit with the only intention of maybe someone helping me with the debug, I know it has a lot of console.logs but they will all be gone soon.

Problem:
img
This is the console.log() I'm using to debug.
console.log('isActive', this.props.reportID, 'fromWhere', this.props.report);

As we can see in these logs (hopefully we can see after some zoom, haha), when a message is sent and I select the report with this new message, the report retrieved from the Onyx store with currentlyViewedReportID does not get updated.
It's probably a problem with the ONYXKEYS.CURRENTLY_VIEWED_REPORTID request.

@lucas-neuhaus-dev
Copy link
Contributor Author

Just realized that the key function on withOnyx takes the prop as input, and we can just destructure the reportID instead of composing another withOnyx on top to get the currentlyViewedReportID.
This solves the problem.

export default withOnyx({
    report: {
        key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
    },
    reportActions: {
        key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
        canEvict: props => !props.isActiveReport,
    },
    session: {
        key: ONYXKEYS.SESSION,
    },
})(ReportActionsView);

Co-authored-by: David Bondy <bondydaa@gmail.com>
Copy link
Contributor

@bondydaa bondydaa left a comment

Choose a reason for hiding this comment

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

I wasn't able to get through all of this but wanted to get these up here before I signed off for an appointment.

I'll finish up later tonight and add any other comments.

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
bondydaa
bondydaa previously approved these changes Mar 2, 2021
src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved
@bondydaa
Copy link
Contributor

bondydaa commented Mar 2, 2021

@roryabraham all yours

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 Outdated Show resolved Hide resolved
roryabraham
roryabraham previously approved these changes Mar 2, 2021
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Okay, good points, thanks for making those small changes. Other than the unused import LGTM 👍

src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved
@marcaaron marcaaron self-requested a review March 2, 2021 17:19
@marcaaron marcaaron merged commit 28910e9 into Expensify:master Mar 2, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 2, 2021
@lucas-neuhaus-dev lucas-neuhaus-dev deleted the add-last-read branch March 2, 2021 17:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a "last unread" indicator for reports
5 participants