feat(comments): allow editing and deleting issue comments #327

jouderianjr · 2017-09-13T01:10:31Z

Closes #24

Before

After

Thanks in advance :)

lex111 · 2017-09-13T07:28:52Z

src/components/comment-list-item.component.js

+    const isActionButtonVisible =
+      comment.user &&
+      authUser.login === comment.user.login &&
+      !Object.prototype.hasOwnProperty.call(comment, 'repository_url');


For what this check?

Because the description of the issue it is also a commentListItem, so I only show the button if is a comment and when is an issue description it has this field (repository_url), does that make sense?

I think I could extract this !Object.prototype.hasOwnProperty.call(comment, 'repository_url');for a function for readability, what do you think?

Still kind of unsure why we need this. Do we need to check for the repository_url field at all?

Also I notice that my initial issue post doesn't seem to allow me to edit:

Not sure if this is where you've set that to happen, but I think it would make sense to show the ActionSheet but only allow for editing for the first issue comment and not deleting. Happy to defer this if you feel like taking care of this in a separate PR however :)

I'm using repository_url for distinguishing between issue description and issue comment, only issue description has this field like I said before I think is better extract that for a method in order to improve the "understanding".

About editing the description, you are completely right, I didn't realize this possibility, I will take care of that in this PR. 😄

lex111 · 2017-09-13T07:30:35Z

src/components/comment-list-item.component.js


          <View style={styles.dateContainer}>
            <Text style={styles.date}>
              {moment(comment.created_at).fromNow()}
            </Text>
          </View>
+
+          {isActionButtonVisible && (
+            <View style={styles.iconContainer}>


Let's for greater clarity, rename the class to something like actionButtonIconContainer?

lex111 · 2017-09-13T07:35:01Z

src/issue/issue.reducer.js

@@ -17,6 +19,8 @@ const initialState = {
  isMerged: false,
  isPendingComments: false,
  isPostingComment: false,
+  isDeletingComment: false,
+  isEditingIssueComment: false,


Maybe better just isEditingComment?

lex111 · 2017-09-13T07:48:28Z

src/issue/screens/issue.screen.js

+    const repoName = repository.name;
+    const owner = repository.owner.login;
+
+    this.props.deleteIssueCommentByDispatch(id, owner, repoName);


I do not know for sure, but it makes sense to define the variable id, if it will be used once, I mean what maybe just pass comment.id?

yeah, you are completely right! 😄

lex111 · 2017-09-13T07:50:21Z

src/locale/languages/en.js

@@ -248,6 +248,12 @@ export const en = {
      areYouSurePrompt: 'Are you sure?',
      applyLabelTitle: 'Apply a label to this issue',
    },
+    comment: {


Please duplicate these all new keys into the remaining language files.

Should I try to translate? or I add the text in English?

Generally not, although if you know any language and can translate, it would be good, but if not, then it is enough to duplicate it in English, otherwise there will be errors (because new keys will not be in the remaining languages)

lex111 · 2017-09-13T07:53:11Z

src/issue/screens/issue.screen.js

+      isPendingIssue ||
+      isDeletingComment
+    );
+    const showLoadingContainer = isPendingComments || isPendingIssue;


Since this variable contains a boolean value, it can be renamed to isShowLoadingContainer?

lex111 · 2017-09-13T08:06:39Z

src/issue/screens/edit-issue-comment.screen.js

+      <ViewContainer>
+        {isEditingIssueComment && <LoadingModal />}
+        <ScrollView>
+          {repository.full_name && (


What do you think, besides the name of the repository, can show the title issue?

Sounds good to me

alejandronanez · 2017-09-13T22:58:54Z

src/issue/issue.action.js

+  return (dispatch, getState) => {
+    const accessToken = getState().auth.accessToken;
+
+    dispatch({ type: DELETE_ISSUE_COMMENT.PENDING });


I know this is not in the scope of this PR but what do you think about using redux-actions instead? We can reduce boilerplate significantly.

You can find and example in this PR #187

If you're not feeling ok with it, that's fine too!

Oh, that's pretty amazing, I really like this approach! But to be honest, I think this should be done in a separated PR, I think is better just refactor the whole issue.action.js instead only this particular part, just for keeping the cohesion of the file.

What do you think about? 😄

Completely agree!

Thanks!!!

alejandronanez · 2017-09-13T23:53:27Z

I completely agree! Thanks!!

…

On Wed, Sep 13, 2017 at 6:42 PM Jouderian Nobre Junior < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/issue/issue.action.js <#327 (comment)>: > @@ -117,6 +121,56 @@ export const postIssueComment = (body, owner, repoName, issueNum) => { }; }; +export const deleteIssueComment = (issueCommentId, owner, repoName) => { + return (dispatch, getState) => { + const accessToken = getState().auth.accessToken; + + dispatch({ type: DELETE_ISSUE_COMMENT.PENDING }); Oh, that's pretty amazing, I really like this approach! But to be honest, I think this should be done in a separated PR, I think is better just refactor the whole issue.action.js instead only this particular part, just for keeping the cohesion of the file. What do you think about? 😄 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#327 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAcYUqKp-g6MwfuwQTHAaadZxv1SeJGBks5siGhtgaJpZM4PVczh> .

andrewda

This is awesome! One thing I think might be improved visually is taking away the ... and replacing it with a long touch. In other words, instead of using up a lot of space with the ellipsis, just use a long tap to bring up the deletion menu.

jouderianjr · 2017-09-15T01:51:10Z

@andrewda yeah, that can be done. To be honest like much this approach because we are maybe "hiding" some features from the user, but I'm not against this approach 😄

I would love to get more opinions about, any thoughts @lex111 @alejandronanez @housseindjirdeh?

jouderianjr · 2017-09-15T03:35:11Z

@lex111 I did most of all improvements, could you take a look? 😄

lex111 · 2017-09-15T12:02:35Z

@jouderianjr now everything is ok 👌

As still a variant with actions, you can look at this https://github.com/AidenMontgomery/react-native-swiping-row

jouderianjr · 2017-09-15T17:09:44Z

Cool, So, now we have basically 3 options :) Which one you guys would prefer?

chinesedfan · 2017-09-16T00:54:17Z

src/issue/screens/issue.screen.js

@@ -240,6 +266,8 @@ class Issue extends Component {
      <CommentListItem
        comment={item}
        onLinkPress={node => this.onLinkPress(node)}
+        onDeletePress={comment => this.deleteComment(comment)}
+        onEditPress={comment => this.editComment(comment)}


I think we can write shorter as onDeletePress={this.deleteComment}. Maybe another pr is required if you guys(@lex111, @jouderianjr) agree with it.

Of course, there are so many places like this. And it doesn't matter.

Yeah, I agree with you, I do prefer the shorter version, I just tried to follow the file pattern, but I also agree in do another PR for that, should be better change all the file.

Yeah I've used arrow syntax within render callbacks throughout the app. Leveraging onDeletePress={this.deleteComment} is better and I think I'll dedicate some time to do that throughout the app

andrewda · 2017-09-16T04:19:57Z

@lex111 😲 Swiping is a really good idea!

jouderianjr · 2017-09-16T04:50:13Z

Cool, 2 votes 😄 I'll implement with swipe action

jouderianjr · 2017-09-16T06:39:09Z

@lex111 @andrewda hey guys, bad news :/ Doing the implementation I realized that we already have swipe on these screens because of tab navigation :( because of that I think that cannot be done.

So, should I implement the long press action?

jouderianjr · 2017-09-16T07:06:28Z

@andrewda I've implemented with long press click. Could you take a look?

lex111 · 2017-09-16T08:33:48Z

@jouderianjr it's a pity, but there is no way to disable swipe on the screen issue?

machour · 2017-09-16T13:48:12Z

I'm having the same issue with TabNavigation swipe that prevents scrolling horizontally in webviews on Android (Readme.md is displayed in webview in #326)

I would love to have the TabNavigation swipe disabled all together in the app. I personally find this type of navigation confusing, especially when you know that you can have the Issue screen (for example) displayed in all 4 tabs at the same time..

jouderianjr · 2017-09-16T14:57:00Z

I think disabling the swipe only in a few screens not intuitive. We could disable for the whole tab navigation, is just set swipeEnabled: false in TabNavigation options, but this is a big change to do, I think we need to get thoughts from the @housseindjirdeh as well :)

housseindjirdeh · 2017-09-17T05:41:03Z

Man thank you all so so much for thinking this through.

So some thoughts:

I highly agree we shouldn't remove swipe on this screen or any screen. I'm actually used to swiping on the left side of the screen to go back and it would feel to weird if some screens didn't allow this. That being said, we could remove it altogether but I would be against this as it feels like a natural way to navigate in my opinion.
That being said, @jouderianjr what do you think of adding the swipe feature and leaving the swipeEnabled for navigation. Are you noticing significant interference? Not sure how exactly that swipe library works, but wondering why it wouldn't be possible to have the swipe throughout the comment list item and only when we swipe on the very very left side of the screen - the navigation swipe kicks in just like the way it is now.

If they both don't seem to coexist together, then we'll have to forego swipe I'm afraid. However, I think this is definitely something we should explore if we can.

If we can't add swipe or this feature, then I'm in favour of long touch and possible including ellipsis as well. From a UI perspective, I think a lot of users might not realize long touch is enabled and I do think a secondary method to open the action sheet would be ideal. Styling-wise, I think it might look better if we slightly reduce the size of the ellipsis icon and have it on the bottom right of the comment list item rather than competing with the time at the top right.

Let me know what you folks think 💬

jouderianjr · 2017-09-17T13:28:48Z

@housseindjirdeh Cool! The swipe feature really improves the UX, I will do more tests in order to add that for the app 😄

vitalkanev · 2017-09-17T18:04:01Z

@housseindjirdeh This screenshot is not from the actual gitpoint app 😂 It's from music app?!

housseindjirdeh · 2017-09-17T18:49:35Z

Haha @vitalkanev I know 😂 😂 It was the first app I can think of (Spotify) that had a really nice swipe feature to remove and add songs to your library. Just wanted to show how even though I can still navigate from swiping the left side of the screen, the swipe still works well in the context of each item.

Probably should have explained why I threw that .gif in 😂

housseindjirdeh · 2017-09-17T18:50:16Z

And thank you @jouderianjr, please keep us posted. If it turns out to not be the smoothest experience, more than happy to use the simpler approach 💃

- Add translation keys for each language. - Show issue name in edit-issue-comment.screen. - Code style minor improvements

- Move action button to bottom. - Add long press to open action menu.

jouderianjr · 2017-09-19T03:36:54Z

Unfortunately, the swipe feature is not possible because of how the tabNavigation is implemented :/

I've updated the code with @housseindjirdeh suggestions, we have long press and the icon button in the bottom

What do you think guys? 😄

Ps. I've attached an incomplete gif haha, The problem happens when we try swipe left from right, because the tabNavigation swipe its global :/

lex111 · 2017-09-19T06:25:33Z

src/components/comment-list-item.component.js

-              />
-            </TouchableOpacity>
-          )}
+      <TouchableWithoutFeedback onLongPress={this.showMenu}>


Maybe use TouchableOpacity better to show feedback?

yeah, looks better!

jouderianjr · 2017-09-21T11:44:15Z

Hey guys, don't forget my PR 🙉 @housseindjirdeh @andrewda

andrewda

😉

housseindjirdeh

This is great @jouderianjr, tested it again and again and editing/deleting work so smoothly. Thank you for this, it's sooo needed in the app.

Just left a few comments here and there for a few things I noticed. Honestly, the majority are minor and I'm happy to help you through each one :)

housseindjirdeh · 2017-09-22T18:23:53Z

routes.js

+  EditIssueComment: {
+    screen: EditIssueCommentScreen,
+    navigationOptions: ({ navigation }) => ({
+      title: navigation.state.params.title,


housseindjirdeh · 2017-09-22T18:28:16Z

src/components/comment-list-item.component.js

@@ -87,6 +89,10 @@ const styles = StyleSheet.create({
  commentRegular: {
    ...regularFont,
  },
+  actionButtonIconContainer: {
+    alignItems: 'flex-end',
+    justifyContent: 'center',


Looks slightly squeezed bottom right, just added a padding: 5 testing on my machine and I think it looks better with that :)

housseindjirdeh · 2017-09-22T18:29:57Z

src/components/comment-list-item.component.js

+    const isActionButtonVisible =
+      comment.user &&
+      authUser.login === comment.user.login &&
+      !Object.prototype.hasOwnProperty.call(comment, 'repository_url');


Still kind of unsure why we need this. Do we need to check for the repository_url field at all?

housseindjirdeh · 2017-09-22T18:54:21Z

src/components/comment-list-item.component.js

-                source={{
-                  uri: comment.user.avatar_url,
-                }}
+      <TouchableOpacity onLongPress={this.showMenu}>


Initially agreed that TouchableOpacity makes sense here, but playing around with on my simulator I'm not sure if we should have this:

Notice how just tapping it while scrolling still shows the opacity change although no action fires until it's actually long pressed. I think this will better as a TouchableWithoutFeedback so we don't get a prompt at all but onLongPress still shows an actionsheet

housseindjirdeh · 2017-09-22T18:58:10Z

src/components/comment-list-item.component.js

+                  <Icon
+                    color={colors.grey}
+                    size={20}
+                    name={'ellipsis-h'}


housseindjirdeh · 2017-09-22T19:03:20Z

src/issue/screens/edit-issue-comment.screen.js

+        {isEditingComment && <LoadingModal />}
+        <ScrollView>
+          {repository.full_name && (
+            <ListItem


Feels kinda out of place to have this ListItem here, I think we can remove this to be honest (the user can easily see the back navigation step with the issue number)

housseindjirdeh · 2017-09-22T19:07:10Z

src/issue/screens/edit-issue-comment.screen.js

+          <SectionList
+            title={translate('issue.newIssue.issueComment', language)}
+          >
+            <TextInput


This is great stuff. The only think I'm noticing is that with super long comments, this TextInput is massive

That blocks the ScrollView scroll capability and hides the the submit button. We'll need to have a fixed height for this input field (or a max height if that's possible - will be better if we can)

One other thing is that it seems slightly squeezed vertically.

A little paddingVertical for the TextInput would be ideal :)

housseindjirdeh · 2017-09-22T19:09:36Z

src/issue/screens/issue.screen.js

@@ -240,6 +266,8 @@ class Issue extends Component {
      <CommentListItem
        comment={item}
        onLinkPress={node => this.onLinkPress(node)}
+        onDeletePress={comment => this.deleteComment(comment)}
+        onEditPress={comment => this.editComment(comment)}


Yeah I've used arrow syntax within render callbacks throughout the app. Leveraging onDeletePress={this.deleteComment} is better and I think I'll dedicate some time to do that throughout the app

housseindjirdeh · 2017-09-22T19:11:39Z

src/components/comment-list-item.component.js

+    const isActionButtonVisible =
+      comment.user &&
+      authUser.login === comment.user.login &&
+      !Object.prototype.hasOwnProperty.call(comment, 'repository_url');


Also I notice that my initial issue post doesn't seem to allow me to edit:

Not sure if this is where you've set that to happen, but I think it would make sense to show the ActionSheet but only allow for editing for the first issue comment and not deleting. Happy to defer this if you feel like taking care of this in a separate PR however :)

jouderianjr · 2017-09-24T15:05:25Z

Hey @housseindjirdeh I set a fixed height to the TextInput, I don't know if that is the best approach :/ Any thoughts about that?

Also, about the "edit issue description" feature I realize that is better handled with that in another PR because I think should be another screen, because we can edit the issue title and the title description, so I think this could be done on the same screen.

housseindjirdeh

I think maxHeight might be a better option 🤔

Don't worry, I can submit a commit for that since it's so minor. All in all it looks amazing mate, more than happy to defer editing the title on a separate PR :)

machour · 2017-09-28T02:10:05Z

src/api/index.js

-    `/notifications?participating=${participating}&all=${all}`,
-    accessToken
-  );
-
 export const fetchMarkNotificationAsRead = (notificationID, accessToken) =>


@housseindjirdeh these were deleted by mistake ⚠️

lex111 reviewed Sep 13, 2017

View reviewed changes

andrewda changed the title ~~Add editing and deleting issue comments feature~~ feat(comments): allow editing and deleting issue comments Sep 13, 2017

alejandronanez reviewed Sep 13, 2017

View reviewed changes

andrewda requested changes Sep 14, 2017

View reviewed changes

chinesedfan reviewed Sep 16, 2017

View reviewed changes

machour mentioned this pull request Sep 16, 2017

feat(markdown): Markdown support #326

Merged

feat(issue-comment): Add delete action

35ca85a

jouderianjr added 3 commits September 19, 2017 00:09

feat(issue_comment): Add edit action (gitpoint#24)

d1a9f43

feat(issue_comment): Improve issue comment action feature (gitpoint#24)

e066981

- Add translation keys for each language. - Show issue name in edit-issue-comment.screen. - Code style minor improvements

feat(issue_comment): Improve usability gitpoint#24

7c92872

- Move action button to bottom. - Add long press to open action menu.

jouderianjr force-pushed the issue_comment_actions_#24 branch from db91839 to 7c92872 Compare September 19, 2017 03:27

lex111 reviewed Sep 19, 2017

View reviewed changes

feat(issue_comment): Use TouchableOpacity for longPress

a07d6ac

lex111 approved these changes Sep 19, 2017

View reviewed changes

andrewda approved these changes Sep 22, 2017

View reviewed changes

housseindjirdeh requested changes Sep 22, 2017

View reviewed changes

andrewda force-pushed the master branch from 0751df7 to cb06e0d Compare September 23, 2017 23:31

andrewda and others added 4 commits September 23, 2017 17:20

Merge branch 'master' into issue_comment_actions_#24

39c6edd

feat(issue_comment): Improve minor UX issues

86d5aef

feat(issue_comment): use bindActionCreators on edit screen

f5257b2

feat(issue_comment): Set fixed height to TextInput

1203618

Merge branch 'master' into issue_comment_actions_#24

a49f978

housseindjirdeh approved these changes Sep 28, 2017

View reviewed changes

housseindjirdeh merged commit 7765b10 into gitpoint:master Sep 28, 2017

machour reviewed Sep 28, 2017

View reviewed changes

jouderianjr mentioned this pull request Sep 30, 2017

v1.3.0 Prep #381

Closed

feat(comments): allow editing and deleting issue comments #327

feat(comments): allow editing and deleting issue comments #327

Conversation

jouderianjr commented Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandronanez commented Sep 13, 2017 via email

andrewda left a comment

Choose a reason for hiding this comment

jouderianjr commented Sep 15, 2017

jouderianjr commented Sep 15, 2017

lex111 commented Sep 15, 2017

jouderianjr commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewda commented Sep 16, 2017 • edited Loading

jouderianjr commented Sep 16, 2017

jouderianjr commented Sep 16, 2017

jouderianjr commented Sep 16, 2017

lex111 commented Sep 16, 2017

machour commented Sep 16, 2017

jouderianjr commented Sep 16, 2017

housseindjirdeh commented Sep 17, 2017

jouderianjr commented Sep 17, 2017

vitalkanev commented Sep 17, 2017

housseindjirdeh commented Sep 17, 2017

housseindjirdeh commented Sep 17, 2017

jouderianjr commented Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jouderianjr commented Sep 21, 2017

andrewda left a comment

Choose a reason for hiding this comment

housseindjirdeh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jouderianjr commented Sep 24, 2017

housseindjirdeh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jouderianjr commented Sep 13, 2017 •

edited

Loading

andrewda commented Sep 16, 2017 •

edited

Loading

jouderianjr commented Sep 19, 2017 •

edited

Loading