Skip to content

Restoring the grouped comments, and fixing the style for it #305

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

Merged
tgolen merged 4 commits into from
Aug 27, 2020
Merged

Restoring the grouped comments, and fixing the style for it #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2946ad9
restoring the grouped comments, and fixing the style for it
sketchydroide Aug 26, 2020
847f563
remove commented line
sketchydroide Aug 27, 2020
ae0db2e
Merge branch 'master' into afonseca_fix_grouping
sketchydroide Aug 27, 2020
f737865
adding a comment and removing the `(`
sketchydroide Aug 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/page/HomePage/Report/ReportHistoryItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class ReportHistoryItem extends React.Component {

return (
<View>
{!displayAsGroup && (<ReportHistoryItemSingle historyItem={historyItem} authToken={authToken} />)}
{!displayAsGroup && <ReportHistoryItemSingle historyItem={historyItem} authToken={authToken} />}
{displayAsGroup && <ReportHistoryItemGrouped historyItem={historyItem} authToken={authToken} />}
</View>
);
Expand Down
17 changes: 12 additions & 5 deletions src/page/HomePage/Report/ReportHistoryItemGrouped.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react';
import {Text} from 'react-native';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import ReportHistoryPropsTypes from './ReportHistoryPropsTypes';
import ReportHistoryItemMessage from './ReportHistoryItemMessage';
Expand All @@ -8,15 +8,22 @@ import styles from '../../../style/StyleSheet';
const propTypes = {
// All the data of the history item
historyItem: PropTypes.shape(ReportHistoryPropsTypes).isRequired,

// Current users auth token
authToken: PropTypes.string.isRequired,
};

class ReportHistoryItemGrouped extends React.PureComponent {
render() {
const {historyItem} = this.props;
const {historyItem, authToken} = this.props;
return (
<Text style={[styles.historyItemMessageWrapper]}>
<ReportHistoryItemMessage historyItem={historyItem} />
</Text>
<View style={[styles.chatItem]}>
<View style={[styles.chatItemRightGrouped]}>
<View style={[styles.chatItemMessage]}>
<ReportHistoryItemMessage historyItem={historyItem} authToken={authToken} />
</View>
</View>
</View>
);
}
}
Expand Down
34 changes: 16 additions & 18 deletions src/page/HomePage/Report/ReportHistoryView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,24 +61,22 @@ class ReportHistoryView extends React.Component {
*/
// eslint-disable-next-line
isConsecutiveHistoryItemMadeByPreviousActor(historyItemIndex) {
// Disable this for now
return false;

// const filteredHistory = this.getFilteredReportHistory();
//
// // This is the created action and the very first action so it cannot be a consecutive comment.
// if (historyItemIndex === 0) {
// return false;
// }
//
// const previousAction = filteredHistory[historyItemIndex - 1];
// const currentAction = filteredHistory[historyItemIndex];
//
// if (currentAction.timestamp - previousAction.timestamp > 300) {
// return false;
// }
//
// return currentAction.actorEmail === previousAction.actorEmail;
const reportHistory = lodashGet(this.props, 'reportHistory', {});

// This is the created action and the very first action so it cannot be a consecutive comment.
if (historyItemIndex === 0) {
return false;
}

const previousAction = reportHistory[historyItemIndex - 1];
const currentAction = reportHistory[historyItemIndex];

// Comments are only grouped if they happen within 5 minutes of each other
if (currentAction.timestamp - previousAction.timestamp > 300) {

@sketchydroide sketchydroide Aug 27, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

why 300, this value seems a bit random, I think we should create a constant for this at some point that has a comment explaining what this means

@tgolen tgolen Aug 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

300 is what the code in web-e has. I usually don't recommend moving things to a constant unless it's a value that changes often, or it is accessed in multiple places, so I'd just leave it as it is for now. The comment you could add might be something like this:

Comments are only grouped if they happen within 5 minutes of each other

@sketchydroide sketchydroide Aug 27, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let me add the comment before you merge

return false;
}

return currentAction.actorEmail === previousAction.actorEmail;
}

/**
Expand Down
8 changes: 8 additions & 0 deletions src/style/StyleSheet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,14 @@ const styles = {
marginRight: 8,
},

chatItemRightGrouped: {
flexGrow: 1,
flexShrink: 1,
flexBasis: 0,
position: 'relative',
marginLeft: 48,
},

chatItemRight: {
flexGrow: 1,
flexShrink: 1,
Expand Down