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

Use outbox messages to render messages to the chat screen #1014

Merged
merged 3 commits into from Aug 13, 2017

Conversation

Projects
None yet
5 participants
@kunall17
Contributor

kunall17 commented Aug 10, 2017

Screenshot:

Last 3 messages are when server not running

@smarx

This comment has been minimized.

smarx commented Aug 10, 2017

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 10, 2017

Changed the look, as showing the elapsed time was of no use as the timestamp was showing that only!

screen shot 2017-08-11 at 12 25 57 am

@borisyankov Updated, ready for review!

@kunall17 kunall17 changed the title from [WIP] Use outbox messages to render messages to the chat screen to Use outbox messages to render messages to the chat screen Aug 10, 2017

@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 10, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

1 similar comment
@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 12, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@borisyankov

Looks pretty decent. Address the comments, and try to make sure there are no perf issues. When I merge the caughtup fix and this, we'll test a bit more and then we can release.

narrow: homeNarrow(),
parsedContent: '<p>Hello</p>',
sender_full_name: 'donald',
timestamp: 1502376058,

This comment has been minimized.

@borisyankov

borisyankov Aug 13, 2017

Contributor

Use simpler values here. For example '1'. Even though the value you used is a more realistic timestamp, it is harder to compare to other values, and it might have a special meaning or not.

@@ -27,10 +35,32 @@ export const getIsFetching = (state: GlobalState): boolean =>
export const getActiveNarrowString = (state: GlobalState): string =>
JSON.stringify(state.chat.narrow);
const getNarrowString = (narrow: Narrow): string => JSON.stringify(narrow);

This comment has been minimized.

@borisyankov

borisyankov Aug 13, 2017

Contributor

I know what you are trying here, but looks like too much abstraction. Just use the JSON.stringify and it will be more clear.

@@ -27,10 +35,32 @@ export const getIsFetching = (state: GlobalState): boolean =>
export const getActiveNarrowString = (state: GlobalState): string =>
JSON.stringify(state.chat.narrow);
const getNarrowString = (narrow: Narrow): string => JSON.stringify(narrow);
const addOutboxMessages = (allMessages, narrow, outboxMessages) => {

This comment has been minimized.

@borisyankov

borisyankov Aug 13, 2017

Contributor

Looking at the getMessagesInActiveNarrow, you probably can put all this code there, as the selector only calls this function.

if (!outboxMessagesForCurrentNarrow) return allMessages[activeNarrowString];
return allMessages[activeNarrowString]
.concat(outboxMessagesForCurrentNarrow)
.sort((a, b) => a.timestamp > b.timestamp);

This comment has been minimized.

@borisyankov

borisyankov Aug 13, 2017

Contributor

This should be a '-' sign.

return getNarrowString(item.narrow) === activeNarrowString;
});
// TODO make this more efficient

This comment has been minimized.

@borisyankov

borisyankov Aug 13, 2017

Contributor

What are the performance concerns you have.
Might be good to address them now.

to: 'Denmark',
type: 'stream',
},
{ timestamp: 1502385930 },

This comment has been minimized.

@borisyankov

borisyankov Aug 13, 2017

Contributor

Here, also use simple timestamps.

const actualState = outboxReducers(initialState, action);
expect(actualState).toEqual(expectedState);
expect(actualState).toEqual(initialState);

This comment has been minimized.

@borisyankov

borisyankov Aug 13, 2017

Contributor

Use 'toBe' here and make sure the state does not mutate.

case EVENT_NEW_MESSAGE: {
const newState = state.filter(item => item.localMessageId === action.localMessageId);
const newState = state.filter(item => item.timestamp !== Number(action.localMessageId));

This comment has been minimized.

@borisyankov

borisyankov Aug 13, 2017

Contributor

Just use a + in front of action.localMessageId to convert to number.

@@ -29,6 +30,13 @@ export const getAccountDetailsUser = createSelector(
},
);
export const getUserByEmail = (users: any[], userEmail: string) =>

This comment has been minimized.

@borisyankov

borisyankov Aug 13, 2017

Contributor

I don't think you need this as a separate function.
Also, I am now moving to extracting these function them to a 'helpers' or similar named files to not be confused with the actual selectors which get only 'state' as a parameter, or are memoized like the createSelector ones.

This comment has been minimized.

@kunall17

kunall17 Aug 13, 2017

Contributor

I have created this function in multiple PR's so lets have this, as it will be used in many places!

@@ -39,6 +39,9 @@ export const specialNarrow = (operand: string): Narrow => [
export const isSpecialNarrow = (narrow: Narrow): boolean =>
narrow.length === 1 && narrow[0].operator === 'is';
export const isNarrowAllPrivate = (narrow: Narrow): boolean =>

This comment has been minimized.

@borisyankov

borisyankov Aug 13, 2017

Contributor

Should the name be isAllPrivateNarrow?

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 13, 2017

@borisyankov done with the changes!

@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 13, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Aug 13, 2017

A PR I just merged did conflict with this. If you rebase again, I think it is ready to merge.
If @Sam1301 has something to say on this code, before I merge, she is welcome to do so :)

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 13, 2017

@borisyankov Done :)

@Sam1301

This comment has been minimized.

Member

Sam1301 commented Aug 13, 2017

(looking at the code)

@borisyankov borisyankov merged commit 8dd2081 into zulip:master Aug 13, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 46.866%
Details
@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Aug 13, 2017

... and it's merged.

@Sam1301

Looks good! 👍 , sorry for the delay, left some minor comments.

ownEmail,
starred,
onLongPress,
isNotYetSent,

This comment has been minimized.

@Sam1301

Sam1301 Aug 13, 2017

Member

couldn't get where isNotYetSent is passed as prop

This comment has been minimized.

@kunall17

kunall17 Aug 13, 2017

Contributor

isNotYetSent is used in the MessageTags

message={message}
twentyFourHourTime={twentyFourHourTime}
ownEmail={message.email}
outbox>

This comment has been minimized.

@Sam1301

Sam1301 Aug 13, 2017

Member

where is outbox prop used?

This comment has been minimized.

@kunall17

kunall17 Aug 13, 2017

Contributor

This should be named as isNotYetSent.
I should stop using find and replace for review changes 😅

I'll address this one in a PR for the action sheet's!

twentyFourHourTime: state.realm.twentyFourHourTime,
}),
boundActions,
)(connectActionSheet(OutboxMessageContainer));

This comment has been minimized.

@Sam1301

Sam1301 Aug 13, 2017

Member

are we using connectActionSheet?

This comment has been minimized.

@kunall17

kunall17 Aug 13, 2017

Contributor

I left it as, I thought I'll use this as menu for having menu's like retry or cancel message send, which should be a part of an outbox!

};
render() {
const { message, auth, actions, twentyFourHourTime, isBrief } = this.props;

This comment has been minimized.

@Sam1301

Sam1301 Aug 13, 2017

Member

this component seems very similar to MessageContainer; maybe we make changes in MessageContainer itself to keep the code DRY?

This comment has been minimized.

@kunall17

kunall17 Aug 13, 2017

Contributor

Initially I did that but it caused a lot of if condition's in the component, hence I decided to make it's own component!

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 13, 2017

Thanks for the review btw @Sam1301 👍

@Sam1301

This comment has been minimized.

Member

Sam1301 commented Aug 13, 2017

Cool, I haven't done a thorough review, will do so latest by tomorrow 👍

@Sam1301

okay, did a somewhat thorough review of this pr and previous one (#977), rest of the changes look good! 👍

@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 14, 2017

Hello @kunall17, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #[issue number]”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

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