feat: add hardcoded creators list with carousel UI #80
Conversation
AbhinavOC24
requested review from
Misofser and
yurijmikhalevich
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR transitions the creators section from a dynamic API-driven approach to a hardcoded list with an improved carousel UI. The change addresses rate-limiting issues with the GitHub API while enhancing the user experience through better navigation controls.
Key changes:
- Replaced dynamic GitHub API fetching with 9 hardcoded creator profiles
- Implemented horizontal carousel navigation with previous/next buttons and smooth scrolling
- Standardized code formatting throughout CSS and JavaScript files
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| main.css | Added carousel styling with navigation buttons, scroll behavior, and responsive container styles; applied formatting improvements |
| index.html | Added 9 hardcoded creator profiles with avatar images and bios; integrated carousel navigation buttons |
| app.js | Commented out dynamic API creator fetching; added initCarousel() function for navigation button event handlers; applied code formatting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Added an initial hardcoded list of creators and implemented a carousel for navigation. Skipped dynamic population of PR mergers from GitHub API for now, since it requires multiple API requests and was hitting rate limits.
|
Pushed a new commit, all Copilot suggestions have been fixed (indentation + missing references). Should be good now. @yurijmikhalevich |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| document.addEventListener('DOMContentLoaded', () => { | ||
| setAuthors(members_url).catch(console.error); | ||
| document.addEventListener("DOMContentLoaded", async () => { | ||
| loadAllContributors(); |
There was a problem hiding this comment.
The event listener callback is marked as async but doesn't await any asynchronous operations. Since loadAllContributors() returns a promise that isn't being awaited, errors from that function won't propagate properly. Either await loadAllContributors() or remove the async keyword from the callback.
| loadAllContributors(); | |
| await loadAllContributors(); |
There was a problem hiding this comment.
I agree that it makes sense to remove async added in this PR.
Also, it could make sense to add .catch(console.error) to loadAllContributors to prevent it crashing from calling initCarousel and setupScrollToTop.
| loadAllContributors(); | |
| loadAllContributors().catch(console.error); |
| if (!persons || !nextBtn || !prevBtn) return; | ||
|
|
||
| nextBtn.addEventListener("click", () => { | ||
| persons.scrollBy({ left: 260, behavior: "smooth" }); |
There was a problem hiding this comment.
The scroll distance of 260 pixels is a magic number. Consider extracting this to a named constant (e.g., SCROLL_DISTANCE) to improve maintainability and make it easier to adjust the scroll behavior.
| }); | ||
|
|
||
| prevBtn.addEventListener("click", () => { | ||
| persons.scrollBy({ left: -260, behavior: "smooth" }); |
There was a problem hiding this comment.
The scroll distance of -260 pixels is a magic number. Consider extracting this to a named constant (e.g., SCROLL_DISTANCE) to improve maintainability and make it easier to adjust the scroll behavior.
There was a problem hiding this comment.
Thank you for cleaning up the formatting in this codebase, but I wish it were a separate PR.
The formatting changes being mixed with your functional changes make this PR less focused and your changes harder to review.
No need to change this in this PR
There was a problem hiding this comment.
I see that it doesn't look good on mobile screens. Can you please fix this?
yurijmikhalevich
left a comment
yurijmikhalevich
left a comment
There was a problem hiding this comment.
Thank you for preparing this PR and for the update! 🙌 There is a bit more to be done here before we can merge it. Please see the comments above and let me know if you have any questions or if I can help!
There was a problem hiding this comment.
Another issue is that clicking the left button when the carousel is in the leftmost position does nothing.
Let's either disable/hide the button when the carousel is in the leftmost position or make the carousel cyclic and infinite.
|
@AbhinavOC24, I am closing this PR because it is stale. Please reopen it if you an update and want to merge it. |
2 participants
Added an initial hardcoded list of creators and implemented a carousel for navigation. Skipped dynamic population of PR mergers from GitHub API for now, since it requires multiple API requests and was hitting rate limits.
How does this PR impact the user?
Before: Creators list was static and janky because of rate limits due to api fetching.
After: Users now see a carousel UI with 9 hardcoded creators, making it easier to browse through them in a visually engaging way.
Description
Added an initial hardcoded list of 9 creators in the #creators section.
Implemented a carousel UI to allow horizontal navigation through the creators.
Skipped dynamic population of PR mergers from the GitHub API (due to multiple requests leading to rate limiting).
Limitations
Dynamic fetching of PR mergers from GitHub API is not implemented in this PR (skipped due to API rate-limit issues).
Only the hardcoded 9 creators are shown.
Checklist