Skip to content
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.

Sign up for GitHub

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

Jump to bottom

update commit list view and add activity view #306

Closed
wants to merge 1 commit into from
Closed

update commit list view and add activity view #306

wants to merge 1 commit into from

Conversation

sahil143
Copy link
Contributor

@sahil143 sahil143 commented Jan 3, 2023 •
edited

Fixes

https://issues.redhat.com/browse/HAC-2545
https://issues.redhat.com/browse/HAC-2817

[TODO]:

  • update the status column commit list view
  • add filter dropdown for status

Description

update commit list view

Type of change

  • Feature
  • Bugfix
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Screen shots / Gifs for design review

Screenshot 2023-01-10 at 7 59 44 PM

Screenshot 2023-01-10 at 8 09 06 PM

Screenshot 2023-01-10 at 8 10 45 PM

Screenshot 2023-01-10 at 8 10 57 PM

Screenshot 2023-01-10 at 8 11 16 PM

How to test or reproduce?

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 3, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 3, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 3, 2023
@sahil143 sahil143 changed the title [WIP] update commit list view and add activity view update commit list view and add activity view Jan 3, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 3, 2023
@rohitkrai03
Copy link
Contributor

@sahil143 Unit tests are failing.

@codecov
Copy link

codecov bot commented Jan 10, 2023 •
edited

Codecov Report

Merging #306 (e7a195f) into main (17b0592) will increase coverage by 0.09%.
The diff coverage is 48.00%.

❗ Current head e7a195f differs from pull request most recent head 9842e6d. Consider uploading reports for the commit 9842e6d to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #306      +/-   ##
==========================================
+ Coverage   74.85%   74.94%   +0.09%     
==========================================
  Files         432      433       +1     
  Lines        9169     9226      +57     
  Branches     2482     2501      +19     
==========================================
+ Hits         6863     6914      +51     
- Misses       2178     2187       +9     
+ Partials      128      125       -3
Impacted Files Coverage Δ
src/components/Commits/CommitsListHeader.tsx 75.00% <ø> (ø)
...mponents/Commits/__data__/pipeline-with-commits.ts 100.00% <ø> (ø)
src/components/Commits/CommitsListView.tsx 18.46% <8.10%> (-10.11%) ⬇️
...nents/PipelineRunListView/PipelineRunsListView.tsx 95.45% <50.00%> (ø)
src/components/Activity/ActivityTab.tsx 77.77% <77.77%> (ø)
src/components/Commits/CommitsListRow.tsx 90.32% <81.81%> (-5.52%) ⬇️
...mponents/ApplicationDetails/ApplicationDetails.tsx 87.93% <100.00%> (-0.21%) ⬇️
src/consts/pipelinerun.ts 100.00% <100.00%> (ø)
src/utils/commits-utils.ts 93.24% <100.00%> (+0.59%) ⬆️
src/components/Components/ComponentListView.tsx 89.28% <0.00%> (-0.72%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17b0592...9842e6d. Read the comment docs.

karthikjeeyar
karthikjeeyar reviewed Jan 11, 2023
View reviewed changes
Copy link
Contributor

@karthikjeeyar karthikjeeyar left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commits are now sorted based on recent activity!

Added some of questions/improvements below:

image

  1. Activity tab should be before components tab.
  2. @MariaLeonova Do we need to truncate the Commit message or wrap into two lines ?
  3. commited at column should be renamed to Latest commit at

Name filter is currently working component name but it should rather work with PR title/commit message.

Name_Filter

src/components/Commits/CommitsListView.tsx Outdated
Comment on lines 115 to 117
<Button variant="link" className="pf-u-pl-sm">
Learn more
Read more
</Button>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Clicking on the Read more link does nothing but I think should open a getting started with commit modal like we have in commit details page, @MariaLeonova Please confirm?

image

@MariaLeonova
Copy link

I agree, the commit / PR message should wrap if it's too long.
We definitely need to embrace the filter by component functionality, but it need to be separate from filter by name. See the suggested design here: doc

@MariaLeonova
Copy link

Regarding the Read more: I believe it should open a side panel with the relevant info, similar to https://quickstarts.netlify.app/in-app-documentation.
Ran has raised the question with the docs team, asking for guidance here.

@MariaLeonova
Copy link

Per @abigaeljamie 's guidance, the Read more here should be removed for now, since there's no content planned for the side panel.
Slack thread.

@rohitkrai03
Copy link
Contributor

Hey, I've merged the PR that fixed the e2e tests - #305. So going forward it would be good to run the e2e tests locally and paste the screenshot here on the PR before merging. Can you do that?

@sahil143
Copy link
Contributor Author

sahil143 commented Jan 16, 2023 •
edited

@rohitkrai03 I ran the e2e tests. For Private repo, tests are failing. This PR doesn't have any changes related to Private repo.

Screenshot 2023-01-16 at 2 09 15 PM

Screenshot 2023-01-16 at 2 12 42 PM

Screenshot 2023-01-16 at 2 15 34 PM

@sahil143
Copy link
Contributor Author

@karthikjeeyar @MariaLeonova made updates as requested

Screenshot 2023-01-16 at 10 20 52 AM

rohitkrai03
rohitkrai03 requested changes Jan 17, 2023
View reviewed changes
integration-tests/plugins/index.ts Outdated Show resolved Hide resolved
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 18, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sahil143
Once this PR has been reviewed and has the lgtm label, please ask for approval from rohitkrai03 by writing /assign @rohitkrai03 in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sahil143
update commit list view and add activity view
9842e6d 
update commit list title

add filter for commit status

fix unit tests for commit list view

wrap commit title text, rename timestamp column, minor fixes

add unit tests for ActivityTab
@sahil143 sahil143 closed this Jan 18, 2023
@sahil143 sahil143 deleted the commit-list-table branch January 18, 2023 07:18
@sahil143
Copy link
Contributor Author

new PR opened #331

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

@karthikjeeyar karthikjeeyar karthikjeeyar left review comments

@rohitkrai03 rohitkrai03 rohitkrai03 requested changes

@debsmita1 debsmita1 Awaiting requested review from debsmita1

@jerolimov jerolimov Awaiting requested review from jerolimov

Assignees

@rohitkrai03 rohitkrai03

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sahil143 @rohitkrai03 @MariaLeonova @karthikjeeyar @openshift-merge-robot