feat: show commit list #917

lorenzo-cavazzi · 2020-04-24T17:45:47Z

Adds all the necessary logic to handle projects commits.

It includes a ProjectCoordinator component to handle storing project data to the global Redux store (#623). This created extra difficulties since we can't easily migrate all the ProjectModel logic at once. To simplify the temporary co-existence, I created a higher order function withProjectMapped to map project data from the global store to single components whenever needed.

The first commit closes #852 by adding a new "Commits" section in the project overview tab, while the second commit re-uses the CommitsView component in the merge request section.
The third commit injects all the commits-related logic in the environments components. We may want to clean up a little bit Notebooks.container.js when branches will be managed by ProjectCoordinator.

Preview: https://lorenzotest.dev.renku.ch
How to test: browse to any project, go to the new "Commits" section on the "Overview" page. Please check also the "Merge request" section and check a notebook in the "File" tab, starting also a new environment from there. This should cover all the relevant changes.

rokroskar · 2020-04-26T22:31:35Z

Is there an issue/epic for this?

lorenzo-cavazzi · 2020-04-28T07:11:10Z

@rokroskar yes, it's #852 . I accidentally wrote twice #623 in the issue description (fixed) but it was in the linked issues

ciyer

Looks great and works very well. And thanks for fixing the small spacing issues in the Merge Request text. 🎉

The list of commits gets truncated at 100. Could you add a link to the bottom of the list:

More than {maxNumberOfCommits}. To see the full project history, view in GitLab.

vfried

It looks really good!

I also have a small details i would change, could you remove the line on top of the beginning of commits?

Here it would be on top of April.... I think with pt-0 inside the card you can make it happen

I would also align the three buttons to the right on the header and add a tooltip to the commits count.

lorenzo-cavazzi · 2020-04-28T17:24:20Z

@ciyer good point! We may want to handle pagination better in the future, but in the meanwhile that message would make this limitation clear.
I added the info inside a <InfoAlert> Component, let me know if you prefer the plain text.

Here is a comparison (first is the current implementation). The final implementation has 100 instead of 10.

lorenzo-cavazzi · 2020-04-28T17:25:16Z

@vfried thanks for the suggestions!

I think the current spacing on the top is in line with the other tabs (see Description and Stats). Furthermore, making the top padding 0 would make the top border look thicker (it's visible in your image).

Im not sure about moving the buttons to the right. I see that it would make the page more similar to the Cards in the Files tab, but it would be awkward to have the number and the refresh button far from the Commits text they qualify.

ciyer · 2020-04-28T21:58:09Z

@ciyer good point! We may want to handle pagination better in the future, but in the meanwhile that message would make this limitation clear.
I added the info inside a <InfoAlert> Component, let me know if you prefer the plain text.

Here is a comparison (first is the current implementation). The final implementation has 100 instead of 10.

Nice! I prefer the second one, the plain text option. The InfoAlert looks a bit too aggressive to my eyes.

lorenzo-cavazzi · 2020-04-29T07:19:08Z

@ciyer done
I updated https://lorenzotest.dev.renku.ch

ciyer

Looks great! Can't wait to have this feature on Renkulab. I pushed two small commits:

Fix a warning about duplicate React component keys
Add padding to the commit list in the merge request view so it looks the same as in the project commit view.

lorenzo-cavazzi · 2020-04-29T13:12:01Z

src/utils/Commits.js

    <SingleCommit
-      key={commit.id ? commit.id : commit.readableDate}
+      key={commit.id ? commit.id : `${i}-${commit.readableDate}`}


the same readableDate shouldn't appear twice. If that happens, it may be a bug in how the date is computed

Using committed_date instead of authored_date seems to be the right thing to do to avoid out of order dates. It's also in line with what GitLab shows

Adds all the necessary logic to handle project's commits: * Add a new tab in project overview to show commits * Create ProjectCoordinator with basic metadata * Create higher order function to easily map project data from global store to React components fix #852

lorenzo-cavazzi · 2020-04-29T13:38:56Z

Switched authored_date for committed_date to keep dates consistency with GitLab and avoid duplicates
Squashed commits and added a reference to the PR to keep the history clean

ciyer

👍

vfried · 2020-04-30T06:58:49Z

Im not sure about moving the buttons to the right. I see that it would make the page more similar to the Cards in the Files tab, but it would be awkward to have the number and the refresh button far from the Commits text they qualify.

The new changes look really good, I agree with you with the buttons and the top padding thing is a small detail. I see inside stats is a similar thing so it's ok if you leave it as it is.

Adds all the necessary logic to handle project's commits: * Add a new tab in project overview to show commits * Create ProjectCoordinator with basic metadata * Create higher order function to easily map project data from global store to React components fix #852

lorenzo-cavazzi · 2020-04-30T08:10:33Z

I had to rebase, could someone please re-approve?

lorenzo-cavazzi requested a review from a team as a code owner April 24, 2020 17:45

ciyer requested changes Apr 28, 2020

View reviewed changes

vfried suggested changes Apr 28, 2020

View reviewed changes

lorenzo-cavazzi force-pushed the 852-commits branch from 92e0069 to 81a09e7 Compare April 29, 2020 07:09

lorenzo-cavazzi added this to the sprint-2020-03-27 milestone Apr 29, 2020

ciyer previously approved these changes Apr 29, 2020

View reviewed changes

lorenzo-cavazzi commented Apr 29, 2020

View reviewed changes

lorenzo-cavazzi added a commit that referenced this pull request Apr 29, 2020

refactor(project): use commit shared component in merge requests (#917)

7b961c2

lorenzo-cavazzi added a commit that referenced this pull request Apr 29, 2020

refactor(environments): use commit data from project coordinator (#917)

4e38152

lorenzo-cavazzi dismissed ciyer’s stale review via 4e38152 April 29, 2020 13:36

lorenzo-cavazzi force-pushed the 852-commits branch from 0587815 to 4e38152 Compare April 29, 2020 13:36

lorenzo-cavazzi requested a review from ciyer April 29, 2020 13:39

ciyer previously approved these changes Apr 29, 2020

View reviewed changes

vfried previously approved these changes Apr 30, 2020

View reviewed changes

lorenzo-cavazzi added 3 commits April 30, 2020 10:04

refactor(project): use commit shared component in merge requests (#917)

4debee8

refactor(environments): use commit data from project coordinator (#917)

e9ca00c

lorenzo-cavazzi dismissed stale reviews from vfried and ciyer via e9ca00c April 30, 2020 08:05

lorenzo-cavazzi force-pushed the 852-commits branch from 4e38152 to e9ca00c Compare April 30, 2020 08:05

lorenzo-cavazzi requested review from vfried and ciyer April 30, 2020 08:10

vfried approved these changes Apr 30, 2020

View reviewed changes

lorenzo-cavazzi merged commit e9ca00c into master Apr 30, 2020

lorenzo-cavazzi deleted the 852-commits branch April 30, 2020 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: show commit list #917

feat: show commit list #917

lorenzo-cavazzi commented Apr 24, 2020 •

edited

Loading

rokroskar commented Apr 26, 2020

lorenzo-cavazzi commented Apr 28, 2020

ciyer left a comment

vfried left a comment

lorenzo-cavazzi commented Apr 28, 2020 •

edited

Loading

lorenzo-cavazzi commented Apr 28, 2020

ciyer commented Apr 28, 2020

lorenzo-cavazzi commented Apr 29, 2020

ciyer left a comment

lorenzo-cavazzi Apr 29, 2020

lorenzo-cavazzi Apr 29, 2020

lorenzo-cavazzi commented Apr 29, 2020

ciyer left a comment

vfried commented Apr 30, 2020

lorenzo-cavazzi commented Apr 30, 2020

feat: show commit list #917

feat: show commit list #917

Conversation

lorenzo-cavazzi commented Apr 24, 2020 • edited Loading

rokroskar commented Apr 26, 2020

lorenzo-cavazzi commented Apr 28, 2020

ciyer left a comment

Choose a reason for hiding this comment

vfried left a comment

Choose a reason for hiding this comment

lorenzo-cavazzi commented Apr 28, 2020 • edited Loading

lorenzo-cavazzi commented Apr 28, 2020

ciyer commented Apr 28, 2020

lorenzo-cavazzi commented Apr 29, 2020

ciyer left a comment

Choose a reason for hiding this comment

lorenzo-cavazzi Apr 29, 2020

Choose a reason for hiding this comment

lorenzo-cavazzi Apr 29, 2020

Choose a reason for hiding this comment

lorenzo-cavazzi commented Apr 29, 2020

ciyer left a comment

Choose a reason for hiding this comment

vfried commented Apr 30, 2020

lorenzo-cavazzi commented Apr 30, 2020

lorenzo-cavazzi commented Apr 24, 2020 •

edited

Loading

lorenzo-cavazzi commented Apr 28, 2020 •

edited

Loading