[Code] Commit component rework #46097

rylnd · 2019-09-19T00:38:58Z

Summary

This is the first chunk of work for the Commit Search feature (elastic/code#1587, elastic/code#1230); I'm at a good stopping point and I wanted to open this up for feedback.

What it does

Moves commit-related components into a commits folder
Breaks commit-related components into separate files
Refactors the existing Commit component to use more EUI components and less custom CSS
Updates the Commit component according to the new designs

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

Add org/repo link (conditionally)
This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

There were already >5 components in this file; this splits them into smaller, distinct components/files and an index.

Rather than keeping them inline.

This is embedded within the Commit component, for now. Obviates CommitLink for now, but we'll address that in the PR.

* Parses and displays the commit title/description * Parses and displays the author and date * Adds new translations

elasticmachine · 2019-09-19T00:39:00Z

Pinging @elastic/code

rylnd

Adding comments/questions for reviewers to respond to.

x-pack/legacy/plugins/code/public/components/commits/commit.tsx

x-pack/legacy/plugins/code/public/components/commits/history.tsx

elasticmachine · 2019-09-19T02:25:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mw-ding

Mostly are good. 2 comments here regarding the links

For the org/repo link, I don't think it's necessary for the commit message item in the source view page, because they are already in the repo. You can add it later when implementing the commit history page. This is the helper function to parse out the org and name from repoUri
The above comment might also be true for the commit sha links on the right. We may want to disable the link but just show as text in the source view page.

What do you think? @rylnd @daveyholler

x-pack/legacy/plugins/code/public/components/commits/commit.tsx

x-pack/legacy/plugins/code/public/components/commits/history.tsx

mw-ding · 2019-09-19T18:13:18Z

For i18n items, we don't need to deal with those json files any more, the kibana UI team will handle that.

See https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/GUIDELINE.md for details.

rylnd · 2019-09-24T20:27:32Z

The above comment might also be true for the commit sha links on the right. We may want to disable the link but just show as text in the source view page.

@mw-ding in my mind, clicking on the sha will bring you to that commit's page, with all the file changes involved. While it doesn't currently do that, (and instead takes you to the history view rooted at that commit, right?) I can see that being useful behavior regardless of which page you're on. Or are you thinking that the route shift as it currently stands is too subtle?

mw-ding · 2019-09-24T21:26:01Z

The above comment might also be true for the commit sha links on the right. We may want to disable the link but just show as text in the source view page.

@mw-ding in my mind, clicking on the sha will bring you to that commit's page, with all the file changes involved. While it doesn't currently do that, (and instead takes you to the history view rooted at that commit, right?) I can see that being useful behavior regardless of which page you're on. Or are you thinking that the route shift as it currently stands is too subtle?

Oh, I am not referring to the View All button, but the hash commit on each commit message item on the source view page.

this one:

What I am saying here is should we consider providing an option in the component to only show commit hash as pure text for the sake of source view page.

mw-ding

LGTM overall. I don't want the last minute discussion to block the progress, particularly having the follow up commit search page.

elasticmachine · 2019-09-24T22:14:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

I was reading the wrong field in figma. Whoops!

If we're already viewing the commit in the context of the repo, the link here isn't as helpful. For our first use, the commit history, that is the case.

We should build a helper for this, but these are strewn about the codebase.

rylnd · 2019-09-25T21:54:40Z

x-pack/legacy/plugins/code/public/components/commits/commit.tsx

+}
+
+const copyLabel = i18n.translate('xpack.code.commits.copyCommitAriaLabel', {
+  defaultMessage: 'Copy the full commit ID',


@mw-ding one more question: who writes/reviews this copy?

Do you mean the default English version here or the japanese and chinese ones? The first one should be ourselves. The latter ones my understanding is the kibana ui team will handle this.

Yeah, I was referring to the default english. I saw the guidelines and nothing looked out of line, but I wasn't sure if there was more process involved and wanted to call it out.

mengwei is right, we need to be responsible for the English version.

also, in Code team, we do Chinese translation (at least for the first round) by ourselves

elasticmachine · 2019-09-25T23:43:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-26T01:11:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

rylnd · 2019-09-26T04:55:15Z

retest

elasticmachine · 2019-09-26T06:59:37Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

Uses Date.UTC to get a non-relative date/time.

elasticmachine · 2019-09-26T17:35:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

rylnd · 2019-09-26T18:32:24Z

retest

rylnd · 2019-09-26T20:04:08Z

@elasticmachine merge upstream

elasticmachine · 2019-09-26T20:39:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mw-ding · 2019-09-26T20:53:46Z

@daveyholler can you help to take a look at this one?

daveyholler · 2019-09-26T21:10:57Z

@mw-ding @rylnd Looking now

elasticmachine · 2019-09-26T21:27:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-30T15:37:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Remove unnecessary whitespace * Split CommitHistory into multiple files There were already >5 components in this file; this splits them into smaller, distinct components/files and an index. * Extract props to interfaces Rather than keeping them inline. * Add basic implementation of new commit actions This is embedded within the Commit component, for now. Obviates CommitLink for now, but we'll address that in the PR. * Update the remainder of the Commit layout * Parses and displays the commit title/description * Parses and displays the author and date * Adds new translations * Update translation keys to match naming standards See https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/GUIDELINE.md for details. * Extract commit message parsing to helper util * Move CommitLink to commit folder * Fix font-sizes of commit text I was reading the wrong field in figma. Whoops! * Add RepoLink component controlled by a boolean If we're already viewing the commit in the context of the repo, the link here isn't as helpful. For our first use, the commit history, that is the case. * Simpler construction of app paths We should build a helper for this, but these are strewn about the codebase. * Update snapshots following text size changes * Truncate summary text * Fix type error on test component * Make date-dependent test timezone-agnostic Uses Date.UTC to get a non-relative date/time.

rylnd added 5 commits September 18, 2019 19:25

Remove unnecessary whitespace

42b0f8e

Split CommitHistory into multiple files

ccee055

There were already >5 components in this file; this splits them into smaller, distinct components/files and an index.

Extract props to interfaces

adc9669

Rather than keeping them inline.

Add basic implementation of new commit actions

ddf180a

This is embedded within the Commit component, for now. Obviates CommitLink for now, but we'll address that in the PR.

Update the remainder of the Commit layout

db1c88b

* Parses and displays the commit title/description * Parses and displays the author and date * Adds new translations

rylnd added v8.0.0 Team:Code release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Sep 19, 2019

rylnd requested a review from a team as a code owner September 19, 2019 00:38

rylnd requested review from mw-ding, WangQianliang and daveyholler September 19, 2019 00:39

rylnd commented Sep 19, 2019

View reviewed changes

mw-ding reviewed Sep 19, 2019

View reviewed changes

x-pack/legacy/plugins/code/public/components/commits/commit.tsx Outdated Show resolved Hide resolved

x-pack/legacy/plugins/code/public/components/commits/commit.tsx Outdated Show resolved Hide resolved

x-pack/legacy/plugins/code/public/components/commits/history.tsx Show resolved Hide resolved

rylnd added 4 commits September 24, 2019 14:24

Merge branch 'master' into search-commit-history

d83693a

Update translation keys to match naming standards

eada6d8

See https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/GUIDELINE.md for details.

Extract commit message parsing to helper util

ef18c07

Move CommitLink to commit folder

9e2387e

mw-ding approved these changes Sep 24, 2019

View reviewed changes

WangQianliang approved these changes Sep 25, 2019

View reviewed changes

rylnd added 3 commits September 25, 2019 15:46

Fix font-sizes of commit text

332a5f8

I was reading the wrong field in figma. Whoops!

Add RepoLink component controlled by a boolean

0315994

If we're already viewing the commit in the context of the repo, the link here isn't as helpful. For our first use, the commit history, that is the case.

Simpler construction of app paths

b81dcb5

We should build a helper for this, but these are strewn about the codebase.

Update snapshots following text size changes

75dfcef

rylnd commented Sep 25, 2019

View reviewed changes

rylnd added 2 commits September 25, 2019 16:56

Truncate summary text

e5785e5

Fix type error on test component

abc958e

Make date-dependent test timezone-agnostic

2738159

Uses Date.UTC to get a non-relative date/time.

Merge branch 'master' into search-commit-history

514cad9

daveyholler approved these changes Sep 26, 2019

View reviewed changes

Merge branch 'master' into search-commit-history

845d456

rylnd merged commit c6c1d8e into elastic:master Sep 30, 2019

rylnd deleted the search-commit-history branch September 30, 2019 15:37

rylnd mentioned this pull request Sep 30, 2019

[7.x] [Code] Commit component rework (#46097) #46940

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Code] Commit component rework #46097

[Code] Commit component rework #46097

rylnd commented Sep 19, 2019 •

edited

Loading

elasticmachine commented Sep 19, 2019

rylnd left a comment

elasticmachine commented Sep 19, 2019

mw-ding left a comment •

edited

Loading

mw-ding commented Sep 19, 2019

rylnd commented Sep 24, 2019

mw-ding commented Sep 24, 2019

mw-ding left a comment •

edited

Loading

elasticmachine commented Sep 24, 2019

rylnd Sep 25, 2019

mw-ding Sep 25, 2019 •

edited

Loading

rylnd Sep 26, 2019

zfy0701 Sep 26, 2019

elasticmachine commented Sep 25, 2019

elasticmachine commented Sep 26, 2019

rylnd commented Sep 26, 2019

elasticmachine commented Sep 26, 2019

elasticmachine commented Sep 26, 2019

rylnd commented Sep 26, 2019

rylnd commented Sep 26, 2019

elasticmachine commented Sep 26, 2019

mw-ding commented Sep 26, 2019

daveyholler commented Sep 26, 2019

elasticmachine commented Sep 26, 2019

elasticmachine commented Sep 30, 2019

[Code] Commit component rework #46097

[Code] Commit component rework #46097

Conversation

rylnd commented Sep 19, 2019 • edited Loading

Summary

What it does

Checklist

For maintainers

elasticmachine commented Sep 19, 2019

rylnd left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 19, 2019

💔 Build Failed

mw-ding left a comment • edited Loading

Choose a reason for hiding this comment

mw-ding commented Sep 19, 2019

rylnd commented Sep 24, 2019

mw-ding commented Sep 24, 2019

mw-ding left a comment • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Sep 24, 2019

💔 Build Failed

rylnd Sep 25, 2019

Choose a reason for hiding this comment

mw-ding Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

rylnd Sep 26, 2019

Choose a reason for hiding this comment

zfy0701 Sep 26, 2019

Choose a reason for hiding this comment

elasticmachine commented Sep 25, 2019

💔 Build Failed

elasticmachine commented Sep 26, 2019

💔 Build Failed

rylnd commented Sep 26, 2019

elasticmachine commented Sep 26, 2019

💔 Build Failed

elasticmachine commented Sep 26, 2019

💔 Build Failed

rylnd commented Sep 26, 2019

rylnd commented Sep 26, 2019

elasticmachine commented Sep 26, 2019

💚 Build Succeeded

mw-ding commented Sep 26, 2019

daveyholler commented Sep 26, 2019

elasticmachine commented Sep 26, 2019

💚 Build Succeeded

elasticmachine commented Sep 30, 2019

💚 Build Succeeded

rylnd commented Sep 19, 2019 •

edited

Loading

mw-ding left a comment •

edited

Loading

mw-ding left a comment •

edited

Loading

mw-ding Sep 25, 2019 •

edited

Loading