-
Notifications
You must be signed in to change notification settings - Fork 69
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
[NEW] LeaderBoard Component #62
Conversation
This pull request introduces 1 alert when merging 1959356 into 8c2999d - view on LGTM.com new alerts:
|
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.
Great work @samad-yar-khan ! 👏
Please add argument for TopN where say if N=10, then the component will change layout and only display the Top 10 ranked in a 1 bootstrap column by 3 bootsrap row format. Only display -- heading, ranking, avatar, and username. (no need for merge request counts etc)
It would be good if the width of the browser is shrunk (or when using mobile browser) that this component will reactively shrink down to a similar format.
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.
Need to make your component re-usable.
app/pages/gsoc/gsoc2022.js
Outdated
} | ||
|
||
export async function getServerSideProps() { | ||
const res = await fetch("https://gsoc.rocket.chat/api/data"); |
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.
No hard-coding of URL for re-usable components. Remember that other community builders will be going to their own (not Rocket.Chat's) leaderboard.
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.
Great work @samad-yar-khan ! 👏
Please add argument for TopN where say if N=10, then the component will change layout and only display the Top 10 ranked in a 1 bootstrap column by 3 bootsrap row format. Only display -- heading, ranking, avatar, and username. (no need for merge request counts etc)
It would be good if the width of the browser is shrunk (or when using mobile browser) that this component will reactively shrink down to a similar format.
Thanks a lot, I will add the feature to display top N, and reducing the number of columns for smaller screens. I will rename the css files as well.
app/pages/gsoc/gsoc2022.js
Outdated
@@ -0,0 +1,91 @@ | |||
import Head from "next/head"; |
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.
Make sure this file's name is the same as the CSS file --- Leaderboard
You can parameterize the Leaderboad component with the gsoc2022 URL.
@Sing-Li I have a slight doubt, before I start programming the component again, just want to be sure if I am doing the right thing. As we want the component to be server rendered, shall I use the new React Server Components , because currently the server side rendering is happening from a |
Forcing workflow run |
7c7c8a2
to
697488b
Compare
@Sing-Li I have added the feature to show top N people and have also made the rest of the columns disappear smaller screen. Demo : Here in the end we can see only 5 users on the table because I changed the 2022-01-30.00-32-44.mp4 |
I don't think so. But the component should be statically generatable! So you should have getInitial.. |
Please make the shrunk component compact. So that one can embed it together with other components on the same page. In the compact mode, it should NOT show the counts (as you already have now). But it should show an extra column with the rank ( 1 to 5 etc) |
This pull request introduces 1 alert when merging 076e7f5 into 7be8290 - view on LGTM.com new alerts:
|
076e7f5
to
6f6850a
Compare
This pull request introduces 1 alert when merging 6f6850a into 7be8290 - view on LGTM.com new alerts:
|
2022-01-30.17-55-36.mp4 |
@Sing-Li To make it easy for community builders to host their own leaderboard and keep everything server rendered, I have used dynamic paths. So all they have to do is add their own community name in the paths, and add the API link to their hosted leaderboard along with their community/organization name. After that their leaderboard will be server-rendered and validated after 10secs to avoid stale data. So community builders will be able to use this leaderboard component pretty easily 😄 , with barely 3-4 lines of code. The method can be seen below. And the leaderBoard will be accessible in the path - /gsoc/gsoc2022/socketChat |
Great idea @samad-yar-khan!! You're very very close. But I do still feel that you're missing our architecture, our "component orientation", and the vital role of the headless CMS (strapi) in this architecture. Yes, let's chat a bit on our Rocket.Chat team channels. |
@Sing-Li Sure , I will join you over there :) |
…d update contributor data
a4da0a5
to
30638d2
Compare
@Sing-Li I am have made two collection ( Communities and GSoCContributors ) in the CMS which have a One-To-Many relationship. It will be easy for community builders to add their own Leader Board. Very easily and can easily fetch the data in NextJS and use the component anywhere. We can currently access different leaderboard using dynamic routes and communityId in query as shown in the above comment. Now the data flows into the application through our CMS. |
@samad-yar-khan this is VERY COOL! I wonder if there is any way to have the cron job "generated" from the code. Nothing urgent, please think about it a bit. One less place to update/worry about = greatly improved developer experience (in this case "community builder experience"). |
@Sing-Li Thanks a lot for you constant support and guidance throughout this PR. I got to learn a lot by building this single component. It was a real fun process and I am grateful for what you guys are doing for developers and guiding rookies like us. I was thinking about how I can improve the developer experience by generating cron jobs through some strapi itself, so its easier for developers to discover and enhance performance. This is what I had in mind :
I will keep both of these in a single PR in the future. Would love to hear your thoughts. |
Proposed changes (including videos or screenshots)
New Server Side Rendered LeaderBoard page and LeaderBoard table component which can be reused.
2022-01-29.02-08-02_Trim.mp4
Issue(s)
Fixes : #31