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

Implement drafts screen #1019

Merged
merged 6 commits into from Sep 2, 2017

Conversation

Projects
None yet
4 participants
@kunall17
Contributor

kunall17 commented Aug 12, 2017

Auto saves the messages on narrow change just like the web app

@smarx

This comment has been minimized.

smarx commented Aug 12, 2017

Automated message from Dropbox CLA bot

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

@zulip zulip deleted a comment from zulipbot Aug 23, 2017

@zulip zulip deleted a comment from zulipbot Aug 23, 2017

@zulip zulip deleted a comment from zulipbot Aug 25, 2017

@borisyankov

Just to remind you, once you continue with this PR, this will not have UI, but will remember the message being written per narrow and restore automatically when you go back to it.

@zulipbot zulipbot added reviewed and removed needs review labels Aug 25, 2017

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 25, 2017

Yeah, will just have a drafts component to be accessed by the settings menu!

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Aug 25, 2017

There is no point in drafts list at all.

@zulipbot zulipbot added needs review and removed reviewed labels Aug 26, 2017

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 26, 2017

Ok, updated!

@@ -134,3 +134,17 @@ export const narrowFromMessage = (message: Message, email: string) => {
return streamNarrow(message.display_recipient);
};
export const compareNarrow = (narrow1: Narrow, narrow2: Narrow): boolean => {

This comment has been minimized.

@borisyankov

borisyankov Aug 27, 2017

Contributor

How is this different than just calling isEqual from lodash? (which is a deep equality)

This comment has been minimized.

@kunall17

kunall17 Aug 27, 2017

Contributor

Yup it does, just to test it works added some test's too!

@@ -138,12 +139,41 @@ export default class ComposeBox extends PureComponent {
this.setState({ message: input });
};
componentWillUnmount() {
const { drafts } = this.props;
const thisNarrow = JSON.stringify(this.props.narrow);

This comment has been minimized.

@borisyankov

borisyankov Sep 1, 2017

Contributor

Use selectors.
We have getActiveNarrow and getActiveNarrowString now.
Then create a memoized selector based on them called getActiveDraft that depends on getActiveNarrowString.

This comment has been minimized.

@kunall17

kunall17 Sep 2, 2017

Contributor

I'll do it here as it will simplify the code, but will they optimize the performance
As all we need to do is a key lookup in the state

Also considering the fact the draft will not be used in other components?

This comment has been minimized.

@borisyankov

borisyankov Sep 2, 2017

Contributor

Yep, just for simplifying the code. Perf difference will be negligible.

@@ -134,3 +135,6 @@ export const narrowFromMessage = (message: Message, email: string) => {
return streamNarrow(message.display_recipient);
};
export const compareNarrow = (narrow1: Narrow, narrow2: Narrow): boolean =>

This comment has been minimized.

@borisyankov

borisyankov Sep 1, 2017

Contributor

You can just use isEqual and not create the compareNarrow. I don't think the abstraction is adding anything in this case (if it was, we could have likely called the function areNarrowsEqual)

@zulipbot zulipbot added reviewed and removed needs review labels Sep 1, 2017

@zulipbot zulipbot added needs review and removed reviewed labels Sep 2, 2017

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Sep 2, 2017

Updated @borisyankov

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Sep 2, 2017

Looks good!

@borisyankov borisyankov merged commit 55f9792 into zulip:master Sep 2, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@zulipbot zulipbot removed the needs review label Sep 2, 2017

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