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.

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

Add pull requests component #92

Merged

Conversation

iam-flo
Copy link
Contributor

@iam-flo iam-flo commented Sep 16, 2024

Motivation

Adds a pull request widget component and the necessary data to display pull requests of a user

Description

  • extended Pull request model and api
  • add widget for pull request

Screenshots (if applicable)

Bildschirmfoto 2024-09-16 um 18 06 58

Checklist

General

  • PR title is clear and descriptive
  • PR description explains the purpose and changes
  • Code follows project coding standards
  • Self-review of the code has been done
  • Changes have been tested locally
  • Screenshots have been attached (if applicable)
  • Documentation has been updated (if applicable)

Client (if applicable)

  • UI changes look good on all screen sizes and browsers
  • No console errors or warnings
  • User experience and accessibility have been tested
  • Added Storybook stories for new components
  • Components follow design system guidelines (if applicable)

Server (if applicable)

  • Code is performant and follows best practices
  • No security vulnerabilities introduced
  • Proper error handling has been implemented
  • Added tests for new functionality
  • Changes have been tested in different environments (if applicable)

@iam-flo iam-flo self-assigned this Sep 16, 2024
@iam-flo iam-flo linked an issue Sep 16, 2024 that may be closed by this pull request
@github-actions github-actions bot added client size:M This PR changes 30-99 lines, ignoring generated files. application-server size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 16, 2024
@iam-flo iam-flo changed the title DRAFT: create component for pull requests(#82) Create component for pull requests(#82) Sep 16, 2024
@FelixTJDietrich FelixTJDietrich changed the title Create component for pull requests(#82) Add pull requests component Sep 16, 2024
@FelixTJDietrich
Copy link
Collaborator

We also have colors defined for border and card that you can make use off

Copy link
Collaborator

@GODrums GODrums left a comment

Choose a reason for hiding this comment

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

I wasn't able to run storybook in the currently version due to a naming conflict of the number/prNumber-variable.

I would recommend that you rebase the branch to develop as it already contains the number-attribute.

@iam-flo iam-flo force-pushed the 82-pull-request-component-for-collaboration-dashboard branch from 51c1dfc to de37865 Compare September 20, 2024 10:15
@iam-flo iam-flo requested a review from GODrums September 20, 2024 11:51
@iam-flo
Copy link
Contributor Author

iam-flo commented Sep 20, 2024

Rework and ready to merge

@FelixTJDietrich
Copy link
Collaborator

It looks like this in the Storybook now:
image
https://66a8981a27ced8fef3190d41-wvdgwzpzof.chromatic.com/?path=/docs/ui-appissuecard--docs

Some points that I noticed, see screenshot:

  • You can remove the App prefix, this is only important for the selector
  • The IssueCard is a component building on the blocks in the UI/ directory, our design system, the component should go under Components/Core/IssueCard for now.
  • pullRequest -> issue
  • You have url but you are not using it, I'd expect that you can click Artemis #12 and it opens to the issue link
  • When I change OPEN to CLOSED nothing changes, it should change to the correct icon with the correct color, open does also not use the correct color
  • We don't need repository.nameWithOwner, repository.defaultBranch, repository.visibility, repository.url in my opinion, just the name, then you also don't need to nest it
  • createdAt could be a dayjs date or so that is correctly formatted in the UI
  • There is the fileDiff icon but it is not in the github red for me, maybe there are some Storybook issues? Also I don't know how to configure it in the props
  • There are two labels: bug and enhancement. I also can't see them in the input control object
  • It would be nicer if you can configure each of the component's properties individually instead of the untyped nested object

Copy link
Collaborator

@FelixTJDietrich FelixTJDietrich left a comment

Choose a reason for hiding this comment

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

Try running npm run storybook and play around with the story a bit if you can configure everything and everything looks right. We try to build dumb components in isolation and use storybook to document it. In the future it would be nice that a student can just click through it and see what components exist and also try out the different properties of the component.

@FelixTJDietrich
Copy link
Collaborator

This is currently the component API:

<app-issue-card
  [pullRequest]="{title: 'Add feature X', number: 12, deletions: 5, url: 'http://example.com', state: 'CLOSED', repository: {name: 'Artemis', nameWithOwner: 'artemis-education/artemis', defaultBranch: 'master', visibility: 'PUBLIC', url: 'http://example.com'}, createdAt: 'Jan 1', pullRequestLabels: {'_constructor-name_':'Set'}, reviews: {'_constructor-name_':'Set'}}"
></app-issue-card>

Maybe we can not pass a complex type there, does it make sense, maybe not? Think like SwiftUI previews, this is basically what Storybook is for us

Copy link
Collaborator

@GODrums GODrums left a comment

Choose a reason for hiding this comment

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

Just merged the changes from develop into the branch so npm run storybook works again and I can give quick feedback :)

UI looks very nice already, would love to start using this in one of the next leaderboard features, such as the user pages.
I can second @FelixTJDietrich's suggestion of having a clickable element to get to the Github PR page directly.

Do let me know if you need help with any elements of Felix's feedback, which aren't addressed yet!

@iam-flo iam-flo force-pushed the 82-pull-request-component-for-collaboration-dashboard branch from dc46977 to 78b426e Compare September 25, 2024 16:57
@iam-flo
Copy link
Contributor Author

iam-flo commented Sep 25, 2024

@GODrums I added a hover effect for the component, so that it is clearly clickable. I. think I implemented all requested changes. If you want to support, you can take a look at making the labels look like from GitHub. Haven't been able to implement that. In the future please use rebase for PRs as this cleans up the git tree. I will squash the commits before merge to clean everything up.

@GODrums
Copy link
Collaborator

GODrums commented Sep 25, 2024

@iam-flo Sorry, I was out of line committing in your PR without asking. I think I also forgot to git pull before merging since I was doing it from mobile.
Our usual convention is that @FelixTJDietrich merges all PRs with squashing, so no need to worry about git trees.

I think there are few smaller QoL things of the feedback left, I'll have a quick look and mark them.

@iam-flo
Copy link
Contributor Author

iam-flo commented Sep 26, 2024

@GODrums thank you for the suggestions 😄

Copy link
Collaborator

@GODrums GODrums left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks for implementing all the suggestions!

@FelixTJDietrich FelixTJDietrich merged commit cec2e16 into develop Sep 28, 2024
5 checks passed
@FelixTJDietrich FelixTJDietrich deleted the 82-pull-request-component-for-collaboration-dashboard branch September 28, 2024 13:39
@FelixTJDietrich
Copy link
Collaborator

LGTM, Thanks :)

@GODrums GODrums mentioned this pull request Oct 2, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application-server client size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pull request component for collaboration dashboard
3 participants