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

Implemented editing message #402

Merged
merged 3 commits into from Jul 11, 2017

Conversation

Projects
None yet
6 participants
@kunall17
Contributor

kunall17 commented Mar 5, 2017

Currently I used onLongPress, ideally this should be place in a menu (ActionSheet for IOS) hence when #378 will be completed i'll create an option there in 04cd5f4 :)

@smarx

This comment has been minimized.

smarx commented Mar 5, 2017

Automated message from Dropbox CLA bot

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

const { auth, message, pushRoute, popRoute } = this.props;
if (isSentByYou(auth, message)) {
pushRoute('');
fetchRawMessage(auth, message.id, (res) => {

This comment has been minimized.

@borisyankov

borisyankov Mar 7, 2017

Contributor

We do use async/await and not callbacks.
It should be in the form of:

const res = await fetchRawMessage(auth, message.id);
@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Mar 7, 2017

Right arrow in the modal title bar is not a standard UI for iOS/Android.
We would have wanted it as a button somewhere, either an icon or a text saying 'Update'.

But, in that specific case, I think we want to implement it as Slack does: the compose box becomes filled with the message text, and then the icon for send optionally might change (or not).

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Mar 7, 2017

The slack one looks really cool, i'll implement something like that!

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Mar 13, 2017

@borisyankov updated the branch,
I have to add a loading message (for fetching raw message and patching message) so for that should I use the routes and the default LoadingScreen or some other dialog (popup) for loading?

Screencast:
ezgif-2-fc72670b3e

@coveralls

This comment has been minimized.

coveralls commented Apr 7, 2017

Coverage Status

Coverage decreased (-1.5%) to 55.54% when pulling c25f27a on kunall17:patch-1 into cce5ee4 on zulip:master.

@zulipbot

This comment has been minimized.

Member

zulipbot commented May 5, 2017

@kunall17, your pull request has developed a merge conflict! Please review the most recent commit (1c201dd) for conflicting changes and resolve your pull request's merge conflicts.

@borisyankov borisyankov referenced this pull request May 7, 2017

Closed

Feature parity with the native Android app #558

2 of 5 tasks complete
@kunall17

This comment has been minimized.

Contributor

kunall17 commented May 7, 2017

@borisyankov Updated the PR!

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented May 8, 2017

Cool. It works reasonably well right now, I'll do a more thorough review of the code itself.
One functional improvement: when the user clicks 'Edit Message' - focus on the ComposeText.

We might also want to add a cancel button. We should discuss it.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented May 8, 2017

And we should either add a progress bar for the update, or do optimistic UI update which @nashvail worked on for the Reactions. We'll need to discuss that too.

@@ -0,0 +1,15 @@
import { apiPatch } from './apiFetch';

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

Would the name of the actual API call be 'updateMessage'?

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

The reason is that 'edit' is more of a continuous action while update sound more like a command. Makes sense?

This comment has been minimized.

@kunall17

kunall17 May 8, 2017

Contributor

Yeah, makes sense

{
content
},
res => res,

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

Use this to return a more concrete field if only one is relevant which likely is the case.
Maybe res.content or even res.content.raw_content?

This comment has been minimized.

@kunall17

kunall17 May 8, 2017

Contributor

The result is not being used anywhere, I'll remove it

@@ -17,5 +17,6 @@ export const IconStream = (props) => <FontAwesomeIcon name="hashtag" {...props}
export const IconPrivate = (props) => <FontAwesomeIcon name="lock" {...props} />;
export const IconPrivateChat = (props) => <IoniconsIcon name="md-text" {...props} />;
export const IconDownArrow = (props) => <IoniconsIcon name="md-arrow-down" {...props} />;
export const IconEdit = (props) => <MaterialIcon name="done" {...props} />;

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

Maybe 'IconOK' or 'IconDone' ?

@@ -51,3 +51,6 @@ export const shouldBeMuted = (msg, narrow, subscriptions, mutes = []): boolean =
return mutes.some(x => x[0] === msg.display_recipient && x[1] === msg.subject);
};
export const isSentByYou = (message, auth): boolean =>
auth.email === message.sender_email;

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

Maybe an overkill to extract that to a function?

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

Would have suggested isSentBySelf as a name, but am suggesting to inline the logic.

This comment has been minimized.

@kunall17

kunall17 May 8, 2017

Contributor

I'll inline the logic!

const isSentByYouAndNarrowed = (message, auth, narrow) =>
isSentByYou(message, auth) && !isHomeNarrow(narrow);
const actionSheetButtons = [

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

Nice, I was thinking to add exactly that, but decided not to for now. Still like it a lot ;)

@@ -17,6 +17,11 @@ const styles = StyleSheet.create({
export default class MessageList extends React.PureComponent {
autoScrollToBottom = false;
constructor(props) {
super();
this.state = { actionSheetButtons: ['', ''] };

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

We should use state sparingly.
I don't like the API of this action-sheet component at all. It almost forces us to use state, but it is not a good approach.

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

Also, note that any state change does a new render. In this case, it is likely not a perf issue, but still annoys me :)

This comment has been minimized.

@kunall17

kunall17 May 8, 2017

Contributor

But here only once the state is changed, so no issues there.
Even I don't like the static way of using the actionsheets I saw there is an issue related to this in their library github page as well.

@@ -21,16 +21,17 @@ const styles = StyleSheet.create({
},
});
export default class SendButton extends React.Component {
export default class SubmitButton extends React.Component {

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

Do we really want to rename the button?
'submit form' vs 'send message'

This comment has been minimized.

@kunall17

kunall17 May 8, 2017

Contributor

Yeah, Send Button suggest's its being only used to send the message, but submit suggest's it's being used to complete a process (send or edit)

handleEdit = async () => {
const { auth, editMessage, cancelEditMessage } = this.props;
if (editMessage.content.raw_content === this.state.text) {

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

Just do a return.
I don't think there is a single person that cares for the error.
This is too much of a 'developer' thinking ;)

try {
await patchMessage(auth, this.state.text, editMessage.id);
} catch (err) {
this.showErrorAlert(err.message, 'Error Occurred');

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

We need a better error message.
If it is a general error I suggest 'Failed to edit message' or 'Error while updating message'.

title,
message,
[{ text: 'OK', onPress: () => {} }],
{ 'cancelable': true } // eslint-disable-line spellcheck/spell-checker

This comment has been minimized.

@borisyankov

borisyankov May 8, 2017

Contributor

Add cancelable to the dictionary?

This comment has been minimized.

@kunall17

kunall17 May 8, 2017

Contributor

Yeah, almost forgot had modified it earlier!

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented May 8, 2017

Overall, the PR looks quite good. 👍

@kunall17

This comment has been minimized.

Contributor

kunall17 commented May 8, 2017

Updated the PR

@jainkuniya

nice work! 👍 found one issue. If user presses back in between editing message, it shows error on reopening app. Expected:- it should cancel editing.
here is short video

@@ -71,6 +82,29 @@ class ComposeText extends React.Component {
});
}
showErrorAlert = (message, title) => {

This comment has been minimized.

@jainkuniya

jainkuniya May 13, 2017

Member

I think we can extract this from here.
we can create a common alert file (in common) which will be helpful in future.

autocomplete: false,
contentHeight: MIN_HEIGHT,
};
}
componentWillReceiveProps(nextProps) {
if (nextProps.editMessage !== this.props.editMessage) {
this.setState({

This comment has been minimized.

@jainkuniya

jainkuniya May 13, 2017

Member

we can pop keyboard here this.textInput.focus();

This comment has been minimized.

@borisyankov

borisyankov May 13, 2017

Contributor

Indeed. Testing on a real device makes it more obvious.

This comment has been minimized.

@kunall17

kunall17 Jun 8, 2017

Contributor

Thanks for the suggestions @vishwesh3 :) 👍

@zulipbot

This comment has been minimized.

Member

zulipbot commented May 22, 2017

Heads up @kunall17, we just merged some commits (latest: d7cfbe5) 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.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jun 8, 2017

Updated, b47c353 adds a new library to prevent the editMessage key to be persisted in the storage hence preventing those bugs, currently I have used a library which people suggested in the redux-persist repo!

@zulipbot

This comment has been minimized.

Member

zulipbot commented Jun 15, 2017

Heads up @kunall17, we just merged some commits (latest: 74b3cb3) 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 borisyankov added this to Missing functionality in Android Public Release Jun 22, 2017

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jun 30, 2017

Rebased

@zulipbot

This comment has been minimized.

Member

zulipbot commented Jul 4, 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 Jul 4, 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 Jul 4, 2017

I'll do another round of code reviews, before you do the rebase again 😄

@@ -58,6 +58,8 @@
"redux-action-buffer": "^1.1.0",
"redux-logger": "^3.0.1",
"redux-persist": "^4.8.2",
"redux-persist-transform-filter": "^0.0.10",
"redux-persist-transform-immutable": "^4.1.0",

This comment has been minimized.

@borisyankov

borisyankov Jul 5, 2017

Contributor

We do not currently use Immutable.js are you sure we need this?

This comment has been minimized.

@kunall17

kunall17 Jul 5, 2017

Contributor

redux-persist-transform-immutable is redundant will remove it, don't remember how it got here!

@@ -0,0 +1,15 @@
import { apiPatch } from './apiFetch';

This comment has been minimized.

@borisyankov

borisyankov Jul 5, 2017

Contributor

You can get this file merged in, before the whole PR.

This comment has been minimized.

@kunall17

kunall17 Jul 5, 2017

Contributor

Lets keep this file in this, as it has been reviewed. I'll make the api calls in another PR from next time!

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 5, 2017

The main thing about this PR that should be improved is how it handles the state of a message being edited.
We can skip the 'filter-transform' library altogether.
We have two better options:

  • have a per-component state of a message being edited (this.setState) somewhere in the ComposeBox
  • have it in the Redux store, but not under chat but under app. The app part, I may rename session is intentionally not being saved and is a temporary state that is not persisted.
title,
message,
[{ text: 'OK', onPress: () => {} }],
{ 'cancelable': true }

This comment has been minimized.

@borisyankov

borisyankov Jul 5, 2017

Contributor

Why ampersands around cancellable?

I don't think cancellable: true is correct, as an error message should not have a cancel option.

This comment has been minimized.

@kunall17

kunall17 Jul 5, 2017

Contributor

cancelable is for the when the user presses outside the alert box, so if a user presses outside of the alert then it can be dismissed!

@@ -0,0 +1,10 @@
import { Alert } from 'react-native';

This comment has been minimized.

@borisyankov

borisyankov Jul 5, 2017

Contributor

This can also be merged separately, before the full PR is ready.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 5, 2017

have a per-component state of a message being edited (this.setState) somewhere in the ComposeBox

I didn't used state's for this because we need to have the editMessage object in the store as the Title component need's this and this is a read only object, so no need to throw this into state.

I'll move the objects to app.session reducer and I need to dispose this part of reducer on app close, for that I need redux-persist-transform-filter is there a way to do it without this library?
I got this library suggestion from the redux-persist github issues section.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 5, 2017

I meant to rename the 'app' reducer to a 'session' reducer, not sure if you are confused or that you understood me.
We are have this: blacklist: ['app', 'presence'], in store.js so any property of the app reducer will not be persisted.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 5, 2017

Yeah i know about the blacklist, but in the current implementation I had to throw away an object from a reducer hence I had to use this library, so i'll save the object in the app reducer hence we can avoid this library

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 7, 2017

Done

@zulipbot

This comment has been minimized.

Member

zulipbot commented Jul 7, 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 Jul 8, 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.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 11, 2017

@borisyankov Rebased!

@borisyankov borisyankov merged commit 5c31a46 into zulip:master Jul 11, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 58.64%
Details
@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 11, 2017

Another epic-length PR merged :)

@borisyankov borisyankov removed this from Missing functionality in Android Public Release Jul 12, 2017

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