[Discussion] ReportScreen 2.0 wish list

[roryabraham]

Coming from this thread.

This issue is really just a placeholder to discuss the current ReportScreen and ReportActionsView implementation. We should list out any problems or things that are confusing and have a discussion about how to implement improvements in a holistic way.

[roryabraham]

getIsReportFullyVisible

App/src/pages/home/report/ReportActionsView.js

Line 262 in 53bc1ea

getIsReportFullyVisible() {

To me this piece of code is a bit confusing because it determines whether or not the ReportActionsView is visible when it's rendering. I wonder if we can clean this up by not rendering ReportActionsView when it's not visible.

openReportIfNecessary

App/src/pages/home/report/ReportActionsView.js

Line 268 in 53bc1ea

openReportIfNecessary() {

• Does not need to be bound to this:

  App/src/pages/home/report/ReportActionsView.js

  Line 84 in 53bc1ea

  this.openReportIfNecessary = this.openReportIfNecessary.bind(this);

• Personally would rather see this logic that calls the OpenReport API implemented at the ReportScreen level to help keep ReportActionsView more focused on rendering logic. I think that the new message indicator does belong in the ReportActionsView however, because that is a front-end-only UI concept that's managed in component state.
• Feels like it might conflict with ReportScreen.fetchReportIfNeeded:

  App/src/pages/home/ReportScreen.js

  Lines 162 to 173 in ecb6019

  	fetchReportIfNeeded() {
  	const reportIDFromPath = getReportID(this.props.route);
  	// It possible that we may not have the report object yet in Onyx yet e.g. we navigated to a URL for an accessible report that
  	// is not stored locally yet. If props.report.reportID exists, then the report has been stored locally and nothing more needs to be done.
  	// If it doesn't exist, then we fetch the report from the API.
  	if (this.props.report.reportID) {
  	return;
  	}
  	Report.openReport(reportIDFromPath);
  	}

• (minor quip) but could be good to standardize on IfNeeded instead of IfNecessary just to promote consistency and stick with simpler language.

ReconnectReport

App/src/pages/home/report/ReportActionsView.js

Lines 184 to 194 in 53bc1ea

	// When returning from offline to online state we want to trigger a request to OpenReport which
	// will fetch the reportActions data and mark the report as read. If the report is not fully visible
	// then we call ReconnectToReport which only loads the reportActions data without marking the report as read.
	const wasNetworkChangeDetected = lodashGet(prevProps.network, 'isOffline') && !lodashGet(this.props.network, 'isOffline');
	if (wasNetworkChangeDetected) {
	if (isReportFullyVisible) {
	this.openReportIfNecessary();
	} else {
	Report.reconnect(this.props.report.reportID);
	}
	}

• Would prefer to see this logic in ReportScreen along with other calls to openReportIfNecessary, so that more report unread logic is consolidated in the ReportScreen.
• We might be able to simplify this logic and make the code a little bit more readable by exposing a Network.onReconnect function from the Network library, rather than relying on prop changes in a component consuming onNetwork. Particularly in this case because it's calling API methods rather than just controlling the UI.

[marcaaron]

Adapting the ReportScreen so that it only handles a single report in it's entire lifecycle

This is kind of related to navigation and movement to the stack navigator. But I think a lot of the confusing logic we have related to transition animations goes away once we are only ever pushing a new isolated report screen onto the navigation stack instead of trying to recycle the component when the active report in the route changes.

Improve sense of "report visibility" and maybe have some global visibility method

We have some pretty weird logic to check if the sidebar is open or closed or what the screen size is. And I think all that goes away once we kill the drawer navigator.

Unread and new marker indicator

It feels like this stuff is could still have some issues after the sequenceNumber deprecation.

react-freeze hack

I think this goes away once we move to the stack nav

Skeleton view container height logic

Something about this seems weird and could be abstracted. We render a view and then set state when it lays out to get the correct height for the skeleton view. I believe there must be a better way to do that.

ReportActionsView has too many responsibilities

I've tried to improve this bit by bit over time - but all the side effects we handle in componentDidUpdate() are pretty confusing in general. I'd love to make this more approachable somehow. I also think this component could be more modular in general. We're setting new markers, tracking scroll position, calling APIs, etc. It's kinda all over the place.

shouldComponentUpdate() performance optimization

App/src/pages/home/report/ReportActionsView.js

Lines 131 to 179 in 3ecec61

	shouldComponentUpdate(nextProps, nextState) {
	if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) {
	this.sortedReportActions = this.getSortedReportActionsForDisplay(nextProps.reportActions);
	this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(nextProps.reportActions);
	return true;
	}
	if (lodashGet(nextProps.network, 'isOffline') !== lodashGet(this.props.network, 'isOffline')) {
	return true;
	}
	if (nextProps.report.isLoadingMoreReportActions !== this.props.report.isLoadingMoreReportActions) {
	return true;
	}
	if (nextProps.report.isLoadingReportActions !== this.props.report.isLoadingReportActions) {
	return true;
	}
	if (nextProps.report.lastReadSequenceNumber !== this.props.report.lastReadSequenceNumber) {
	return true;
	}
	if (nextState.isFloatingMessageCounterVisible !== this.state.isFloatingMessageCounterVisible) {
	return true;
	}
	if (nextState.newMarkerSequenceNumber !== this.state.newMarkerSequenceNumber) {
	return true;
	}
	if (this.props.isSmallScreenWidth !== nextProps.isSmallScreenWidth) {
	return true;
	}
	if (this.props.isDrawerOpen !== nextProps.isDrawerOpen) {
	return true;
	}
	if (lodashGet(this.props.report, 'hasOutstandingIOU') !== lodashGet(nextProps.report, 'hasOutstandingIOU')) {
	return true;
	}
	if (this.props.isComposerFullSize !== nextProps.isComposerFullSize) {
	return true;
	}
	return !_.isEqual(lodashGet(this.props.report, 'icons', []), lodashGet(nextProps.report, 'icons', []));
	}

I'm not really sure how much of an optimization this is at this point - but it's hard to understand why we are doing all of this and leads to people whitelisting every piece of state or props anyway. It feels kind of anti-patterny and we probably just organize things better.

[MelvinBot]

@roryabraham, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.