Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Leaderboard component and homepage #78
Leaderboard component and homepage #78
Changes from all commits
c5b2c2c
b2d5b26
1806b17
6f7424a
199278e
72850f8
6fbea2b
fe4c6ea
b9ab354
8f09e20
b681b82
80d092a
7ff900b
fb5d518
f680230
f44820d
81a8973
eee3cd7
3937532
97ce98e
d207561
1916d90
6369b5d
2775552
70e1c65
407dd8e
06a5206
e82dc6a
aa6a2e7
7472b20
555e7a7
5745b82
8045967
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤔