-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add pull requests component #92
Conversation
...-server/src/main/java/de/tum/in/www1/hephaestus/codereview/pullrequest/PullRequestLabel.java
Show resolved
Hide resolved
webapp/src/app/ui/pull-request-widget/pull-request-widget.component.html
Outdated
Show resolved
Hide resolved
webapp/src/app/ui/pull-request-widget/pull-request-widget.component.html
Outdated
Show resolved
Hide resolved
webapp/src/app/ui/pull-request-widget/pull-request-widget.component.html
Outdated
Show resolved
Hide resolved
webapp/src/app/ui/pull-request-widget/pull-request-widget.component.html
Outdated
Show resolved
Hide resolved
webapp/src/app/ui/pull-request-widget/pull-request-widget.component.ts
Outdated
Show resolved
Hide resolved
webapp/src/app/ui/pull-request-widget/pull-request-widget.stories.ts
Outdated
Show resolved
Hide resolved
We also have colors defined for border and card that you can make use off |
There was a problem hiding this 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.
...ation-server/src/main/java/de/tum/in/www1/hephaestus/codereview/pullrequest/PullRequest.java
Outdated
Show resolved
Hide resolved
51c1dfc
to
de37865
Compare
Rework and ready to merge |
It looks like this in the Storybook now: Some points that I noticed, see screenshot:
|
There was a problem hiding this 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.
This is currently the component API:
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 |
There was a problem hiding this 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!
dc46977
to
78b426e
Compare
@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. |
@iam-flo Sorry, I was out of line committing in your PR without asking. I think I also forgot to I think there are few smaller QoL things of the feedback left, I'll have a quick look and mark them. |
@GODrums thank you for the suggestions 😄 |
There was a problem hiding this 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!
LGTM, Thanks :) |
Motivation
Adds a pull request widget component and the necessary data to display pull requests of a user
Description
Screenshots (if applicable)
Checklist
General
Client (if applicable)
Server (if applicable)