feat(commit): view commits #258

Antoine38660 · 2017-08-15T21:32:13Z

Fix #5

create a commit screen
- have the commiter name clickable (to open a profile)
have commits that show in the notifications screen be clickable
add number of commits in the repository screen - also clickable
have the number of commits when viewing the diff of a pull request be clickable

andrewda

Wow this is great! Just a super minor change from my first pass through.

andrewda · 2017-08-15T23:01:56Z

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

+        containerStyle={styles.listItemContainer}
+        title={commit.message}
+        titleNumberOfLines={1}
+        subtitle={commit.sha.substring(0, 7) + ' - ' + commit.author.name || ''} // eslint-disable-line prefer-template


Probably may as well use a template here if we have to disable the eslint rule anyways.

housseindjirdeh · 2017-08-16T03:31:02Z

This is coming together nicelyyyy, can't wait to have it included 😍

Antoine38660 · 2017-08-21T18:27:55Z

src/repository/screens/commit.screen.js

+    const { navigation, getCommitDetailsByDispatch } = this.props;
+    const commit = navigation.state.params.commit;
+
+    getCommitDetailsByDispatch(commit);


@housseindjirdeh @andrewda I call this getCommitDetailsByDispatch function in the componentDidMount(). But I have the feeling that the isPendingCommit and isPendingDiff are not to false when the render() function is called.... (And I get an error when I try to render the diff).

Do you know why this happen? I'm stuck since few hours :(

Hmmm so looking through what you have it seems like your actions are changing isPendingCommit and isPendingDiff from true --> false and vice-versa just fine. Have you used logging (redux-logger or reactotron) to make sure the actions were being fired correctly? Also what error message are you seeing?

Will be happy to pull this down tomorrow and see if I can reproduce what you're experiencing 🙌

I'm using reactotron, and the action isn't fired....
The error message is:

undefined is not an object (evaluating commit.commit.message) - commit.screen.js:124

If you try today, you have to find a push event in the EventScreen and tap on master.

Just found a problem, I have:

props: { navigation: Object, commit: Object, diff: string, isPendingCommit: boolean, isPendingDiff: boolean, getCommitDetailsByDispatch: Function, language: string, };

The diff props is affected as {} instead of '' (empty string)!
It's linked to state.repository.diff(in the mapStateToProps function) which is, initially, an empty string.

My question is now: Why this diff props isn't an empty string at the initialization of my screen?

So sorry for not getting on this sooner to help you with debugging 😞 Will make sure to do it within this week, apologies again mate!

alejandronanez · 2017-09-01T12:49:21Z

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

+import { ListItem } from 'react-native-elements';
+import { colors, fonts } from 'config';
+
+type Props = {


I think this should not go here since we're not using @flow. Thoughts @Antoine38660 @housseindjirdeh ?

Nope quite a few components are using flow types :) I think it's definitely nice to have and hopefully we can increase type coverage throughout the app.

But, we don't have @flow on top...so flow should not be checking anything. Or am I just missing something here?

alejandronanez · 2017-09-01T12:50:14Z

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

+
+export const CommitListItem = ({ commit, navigation }: Props) =>
+  <TouchableHighlight
+    onPress={() =>


Can you please move this arrow function out of this prop? Maybe you can create a function outside of the component and call it here instead.

Nice I'm fine either way honestly, but I do see the benefit of having methods separate 👍

Having an arrow function inside a prop may become a perf issue.

alejandronanez · 2017-09-01T12:51:13Z

src/repository/repository.action.js

+
+export const getCommitDetails = commit => {
+  // eslint-disable-next-line no-unused-vars
+  return (dispatch, getState) => {


You can omit getState here.

Thanks @alejandronanez for your review 😃

lex111 · 2017-09-22T10:37:17Z

@Antoine38660 I tried to look, there is an error when clicking on the branch in the event push event:

Antoine38660 · 2017-09-22T12:57:55Z

@lex111 I just tried with your last commit on gitpoint:

And it worked:

Do you remember on which commit you clicked? Maybe it's a non-tested commit content 😄

lex111 · 2017-09-22T13:18:50Z

@Antoine38660 I now tried to open commits for two events and I got the same error.

And if you open push events not for the GitPoint repository, can this be the case?

Antoine38660 · 2017-09-22T15:26:39Z

Atm my events list is flooded by GitPoint events 😅
I'm now watching Webiny/Webiny so I will test ASAP !

Thanks for reporting this error. Did you tried to view a commits list from a Repo view?

lex111 · 2017-09-22T17:02:06Z

A list of commits is displayed:

But a detailed commit shows an error:

Repo - https://github.com/wikimedia/wikipedia-ios

* 2 new routes and screens => CommitList and Commit (WIP) * PushEvent are clickable in the EventScreen

* Commit Screen * Commit diff is now retrieved`and displayed

No more // eslint-disable-next-line no-unused-vars

lex111 · 2017-09-25T08:16:42Z

@Antoine38660 I looked at the new changes, commits from the feed screen are opened. If more than one commit was pushed, will the last commit be displayed? Can make mapping commits, like on GitHub?

lex111 · 2017-09-25T08:18:36Z

src/repository/screens/commit.screen.js

+        <View style={styles.header}>
+          <Text>
+            {translate('repository.commit.byConnector', language, {
+              contributor: committer,


Can we make a clickable committer? To open a profile.

Good idea! I add it to the checklist 😃👍

Antoine38660 · 2017-09-25T08:39:48Z

@lex111 If there are more than one commit pushed, you'll be redirected to a "commit list screen" instead of "Commit screen".
I think it's the better way to display it without alter the feed screen : think in case there are more than 2 commits, the feed will be ugly.

lex111 · 2017-09-25T08:56:59Z

@lex111 If there are more than one commit pushed, you'll be redirected to a "commit list screen" instead of "Commit screen".

Strange then, I have opened the last commit.

Antoine38660 · 2017-09-26T12:13:19Z

@lex111 you were right, I was doing a bad condition : https://github.com/Antoine38660/git-point/blob/feature/commits/src/auth/screens/events.screen.js#L471 (It was > 2)

I fixed that in my last commit.

housseindjirdeh

Just wanted to check in and see if you wanted to get this in as part of the next release? @Antoine38660 If not, no worries at all!

Noticed that I don't see commits list item in the repository screen:

Forgive me if this is something that's still being worked on however 🙌

housseindjirdeh · 2017-09-30T23:36:09Z

src/components/issue-description.component.js

+  ActivityIndicator,
+  TouchableHighlight,
+} from 'react-native';
+import { ListItem, Button } from 'react-native-elements';


I'm sorry, I was fixing merge conflicts here and included this Button as part of the conflict fix. Shouldn't be here since we're leveraging @machour's <Button component. My apologies 😞

Np mate, thanks for the conflict fix 😄

Antoine38660 · 2017-10-01T00:02:37Z

Looks like some "merge from master" broke my code. I'll update it with a correct rebase, then the commit list item should reappear !

Edit: I really want to merge this one, but recent changes broke some parts of this branch.. Moreover I think it'll not be ready for monday 😞

housseindjirdeh · 2017-10-01T01:44:50Z

@Antoine38660 No worries. Apologies are partially my fault, I should have helped you get this off the ground sooner.

But not a problem, we'll be doing more frequent releases after Monday so this shouldn't be out soon after :)

Antoine38660 · 2017-10-05T18:00:20Z

I hope I fixed all the issues introduced by some refactor of our code during last weeks :)

Can you test and let me know your point of view about this feature (and design)?!
Thanks mates!

lex111 · 2017-10-08T07:20:39Z

@Antoine38660

It's strange, for some reason I have a very very long time loading a screen with a commit

On the commit screen, the committer and files are not clickable

From the commits screen, if you switch to a commit, the commit hash is not displayed

housseindjirdeh · 2017-10-19T00:49:05Z

Friendly ping @Antoine38660, just wanted to see if you were stuck or anything at all we can do to help <3

Antoine38660 · 2017-10-19T16:11:20Z

I would like finish once and for all this PR! But I'm facing a lack of time at the moment because of my school courses and school project... I'll be on holidays at the beginning of November so I'll kill this one! 😃
Sorry for the time taken...

Thanks @lex111 for your comment and bugs you found ❤️ ❤️

housseindjirdeh · 2017-10-20T00:58:39Z

@Antoine38660 not at all!

I only asked because I was worried if you were just stuck on something. Please take as long as you need my friend. Please don't let this take up your holiday time too :O Holidays are for relaxation time 🌴

Please don't feel rushed to get on this at anytime 🙏

coveralls · 2017-10-24T19:22:09Z

Coverage decreased (-0.2%) to 30.487% when pulling 94cbfd4 on Antoine38660:feature/commits into 0f7accc on gitpoint:master.

housseindjirdeh · 2018-03-27T00:13:21Z

One more friendly ping @Antoine38660. Do you think you've added most of the final functionality in your recent commits? If so, I can pull this down, finish off what's left and have this merged in :)

machour · 2018-04-23T10:14:52Z

@Antoine38660 if it's okay with you, I propose we close this PR as a lot of internals changed since it was opened. We will scavenge the UI bits from this PR.

Antoine38660 · 2018-04-25T12:23:07Z

Yes @machour it's ok and it isn't a problem for me.
@housseindjirdeh sorry I did'nt see your comment... I'm sorry for the project, because a lack of time, I wasn't able to finish this one 😞

machour · 2018-04-26T12:28:59Z

@Antoine38660 no worries at all dude! Hope you'll find some time during your next summer vacations, there's still sooo much to do 😄

Closing this PR.

* feat(commits): migrate #258 changes * feat(commits): fix i18n * fix(issue): fix issue description styles * feat(commit): Show author and committer, both clickable * chore(i18n): update translation files

Antoine38660 added the 🏗️ work in progress label Aug 15, 2017

Antoine38660 force-pushed the feature/commits branch from b3afdab to 6d5b82a Compare August 15, 2017 21:41

andrewda requested changes Aug 15, 2017

View reviewed changes

Antoine38660 force-pushed the feature/commits branch from 6d5b82a to a7dec31 Compare August 16, 2017 14:24

andrewda approved these changes Aug 19, 2017

View reviewed changes

Antoine38660 commented Aug 21, 2017

View reviewed changes

andrewda mentioned this pull request Aug 22, 2017

Commented on commit event does not contain link to commit #270

Open

alejandronanez reviewed Sep 1, 2017

View reviewed changes

Antoine38660 added 6 commits September 23, 2017 17:51

feat: Show commit

9641e6f

* 2 new routes and screens => CommitList and Commit (WIP) * PushEvent are clickable in the EventScreen

feat: Show commit

a5daf02

* Commit Screen * Commit diff is now retrieved`and displayed

feat(commit): show commits in repository screen

d726352

fix(commit): variables name

fafb8c7

fix(commit): no more error when open a commit (ugly)

867e45a

fix(commit): remove unused params

3c48cad

No more // eslint-disable-next-line no-unused-vars

Antoine38660 force-pushed the feature/commits branch from bbd74ce to 3c48cad Compare September 23, 2017 15:55

Antoine38660 added 2 commits September 23, 2017 18:03

fix(commit): missed <<< HEAD during the last rebase

7173eb6

fix(commit): detailed commit error

9c3e9c7

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

Merge branch 'master' into feature/commits

e4a0533

lex111 reviewed Sep 25, 2017

View reviewed changes

feat(commit): number of commits when viewing a PR is clickable

e8e079b

andrewda and others added 3 commits September 26, 2017 11:26

Merge branch 'master' into feature/commits

1a7b810

Merge branch 'master' into feature/commits

0ec5a59

Merge branch 'master' into feature/commits

e5a57e5

housseindjirdeh reviewed Sep 30, 2017

View reviewed changes

Antoine38660 added 2 commits October 5, 2017 19:53

fix(commits): fix bugs introduced in last merges

1f2fb2c

Merge branch 'master' into feature/commits

d0c8c4e

brandly mentioned this pull request Oct 8, 2017

feat: Issue Events #438

Merged

Merge branch 'master' into feature/commits

94cbfd4

Antoine38660 added 3 commits January 16, 2018 16:56

fix(test): Fix broken TravisCI tests

7ae9e85

feat: commit (merge master)

485cf83

fix(commit): Fix diff url for a commit

365cae5

machour closed this Apr 26, 2018

chinesedfan added a commit that referenced this pull request Nov 16, 2018

feat(commits): migrate #258 changes

fe4f685

chinesedfan added a commit that referenced this pull request Nov 18, 2018

feat(commits): migrate #258 changes

0eacf42

chinesedfan mentioned this pull request Nov 18, 2018

feat(commit): view commits #851

Merged

feat(commit): view commits #258

feat(commit): view commits #258

Conversation

Antoine38660 commented Aug 15, 2017 • edited

andrewda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

housseindjirdeh commented Aug 16, 2017

Antoine38660 Aug 21, 2017 • edited

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

lex111 commented Sep 22, 2017

Antoine38660 commented Sep 22, 2017

lex111 commented Sep 22, 2017

Antoine38660 commented Sep 22, 2017

lex111 commented Sep 22, 2017

lex111 commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Antoine38660 commented Sep 25, 2017

lex111 commented Sep 25, 2017

Antoine38660 commented Sep 26, 2017

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

Antoine38660 commented Oct 1, 2017 • edited

housseindjirdeh commented Oct 1, 2017

Antoine38660 commented Oct 5, 2017 • edited

lex111 commented Oct 8, 2017

housseindjirdeh commented Oct 19, 2017

Antoine38660 commented Oct 19, 2017

housseindjirdeh commented Oct 20, 2017

coveralls commented Oct 24, 2017

housseindjirdeh commented Mar 27, 2018 • edited

machour commented Apr 23, 2018

Antoine38660 commented Apr 25, 2018

machour commented Apr 26, 2018

Antoine38660 commented Aug 15, 2017 •

edited

Antoine38660 Aug 21, 2017 •

edited

Antoine38660 commented Oct 1, 2017 •

edited

Antoine38660 commented Oct 5, 2017 •

edited

housseindjirdeh commented Mar 27, 2018 •

edited