Leaderboard component and homepage #78
Conversation
…1intum/Hephaestus into feature/schema-update-reviews
GODrums
added
enhancement
github-actions
bot
added
application-server
size:XL
github-actions
bot
added
size:XXL
github-actions
bot
removed
the
size:XXL
github-actions
bot
added
size:XL
github-actions
bot
added
size:L
server/application-server/src/main/java/de/tum/in/www1/hephaestus/codereview/user/UserType.java
Show resolved
Hide resolved
| import { TableComponent } from 'app/ui/table/table.component'; | ||
| import { lastValueFrom } from 'rxjs'; | ||
|
|
||
| const defaultData: LeaderboardEntry[] = [ |
There was a problem hiding this comment.
Hmm, I think the way to do it would be mocking the service for the storybook story somehow.
There was a problem hiding this comment.
This data should be part of the story though
There was a problem hiding this comment.
We can probably just delete this data set as it should only be for testing purposes. And then only display the leaderboard if we are able to fetch data from the server. The story currently uses its own testing data set.
Would that solve the issue for you?
There was a problem hiding this comment.
I mean the story fails to fetch the data and then falls back to this data, either remove it here or move it to the story.
webapp/src/app/components/leaderboard/leaderboard.component.html
Outdated
Show resolved
Hide resolved
| const meta: Meta<LeaderboardComponent> = { | ||
| title: 'Components/Leaderboard', | ||
| component: LeaderboardComponent, | ||
| tags: ['autodocs'] |
There was a problem hiding this comment.
I think we would somehow inject a provider for a mocked LeaderboardService that returns the mock data here. Alternatively, we would dumb down the leaderboard component into for example LeaderboardComponent or page (smart with service) and LeaderboardTableComponent (dumb no service).
There was a problem hiding this comment.
For the icon components we would also want stories. Maybe just a big story with all custom icons 🤔 But we can also do this in a follow-up
| @@ -0,0 +1,55 @@ | |||
| <div class="flex flex-col gap-4 py-8"> | |||
| <h1 class="text-3xl font-bold">Artemis Leaderboard</h1> | |||
There was a problem hiding this comment.
Maybe just have a leaderboard page component that is the smart component and separate out the table, I think this is the way.
We can skip having a story for the smart components for now, there we would need to mock the services in the stories.
There was a problem hiding this comment.
Right now the leaderboard is already basically just a wrapper for the table-component with the required logic it feels like.
I guess we could also just move the heading to the main-component to make the separation more clear?
Is there an advantage to separate components I'm missing here?
There was a problem hiding this comment.
I'm still thinking about it 🤔 Okay maybe taking a step back this page would be called the home page or the dashboard page which fetches the data from the service(s) using a leaderboard component. But ideally the leaderboard component is dumb and does not require a dependency to a service, because this always makes testing or showing it in the storybook harder.
If we were to add for example some user actions for filtering the timeframe etc. the leaderboard component would have a callback/output/binding to do that but does not smartly also set the filter for the service. Does this make sense?
In the design system /ui the components should be rather generic and reusable. Having a more specialized component building the leaderboard using the code from the design system which also has a story also makes sense to me. I would like to show the leaderboard component in the storybook for documentation but we do not necessarily have to show the whole home page / dashboard page smart component in there too.
There was a problem hiding this comment.
I think I understand where you are going with that idea now. I personally didn't think about that separation right away because we don't really plan on creating multiple leaderboards rather than one for all repos.
With your explanation I do see the point for it, although I feel like it creates an (unnecessary?) level of abstraction in this use case. Let's further discuss this in our next meeting maybe and rework in a follow-up PR?
There was a problem hiding this comment.
Let's do it in a follow-up. In my mind the leaderboard is just one component on the page. We will add more gamification elements on the same page but you are also right that we might create a leaderboard route per repo in the near future 🤔
webapp/src/app/components/leaderboard/leaderboard.component.html
Outdated
Show resolved
Hide resolved
...plication-server/src/main/java/de/tum/in/www1/hephaestus/leaderboard/LeaderboardService.java
Show resolved
Hide resolved
github-actions
bot
added
size:XL
Motivation
As our first gamfication component, we want to create a rework of the activity leaderboard in Artemis.
Pre-requisits:
Description
This PR adds a simple Artemis leaderboard to the homepage and creates components with corresponding Storybook-Stories.
In the future, it can easily extended with
@tanstack/angular-tablefor filters, sorting, pagination.Screenshots (if applicable)
Checklist
General
Client (if applicable)