-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
✅[outreachy] Registry : Add to favorites #5447 #5451
Conversation
Asked to merge during PR checks.
@mailsg, I don't have a registration from you as a contributor in the outreachy platform. Since I have closed the project, I will close your isse and PR. Thank you for participating in outreachy! |
@mailsg please make sure that you register with the outreachy platform after that I will provide a review |
thanks for registering your contributions in the outreachy platform. Following up with my previous feedback: I see that I can add favorites, have you planned any way that I can use the favorites, e.g. only see the favorites or any other way? |
Hi @svrnm, In addition to providing users with the ability to add and remove items from their favorites, there are several practical ways to make this feature more useful. These can be the planned usage of the
To conclude, the Thanks. |
Please implement "Filter by Favorites". Thanks. |
merged the main branch to implement filter by favorites.
Description
Technical details
Thanks. |
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.
overall lgtm, a few comments
.htmltest.yml
Outdated
- ^blog/(\d+/)?page/\d+ | ||
# TODO drop next line after https://github.com/open-telemetry/opentelemetry.io/issues/5423 is fixed for ja pages: | ||
- ^ja/docs/concepts/instrumentation/libraries/ | ||
# TODO drop next line after https://github.com/open-telemetry/opentelemetry.io/issues/5423 is fixed for pt pages: | ||
- ^pt/docs/concepts/instrumentation/libraries/ | ||
- ^blog/(\d+/)?page/\d+ |
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.
please revert this change
assets/js/addToFavorites.js
Outdated
} else { | ||
favorites.push(registryId); | ||
localStorage.setItem('favorites', JSON.stringify(favorites)); | ||
button.innerHTML = '<i class="fa-solid fa-star"></i>'; |
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.
Remove from favorites to make it clear that it is a favorite and can be removed by clicking on the star once again
merge main to update the brach for implementing suggested review
merge with main to update the branch.
Hi @svrnm, I have made the suggested changes. Thanks. |
I consider this as done! After the outreachy application phase we will take another look and see if we can merge this PR. Thank you! |
I will close this for now, since I raised a follow up discussion here: #5759 if you are interested in continuing contributing, please take a look! If we agree to build this, you can give it a try! Thanks |
Fixes
Description
favorite-btn
that updates dynamically.Technical details
Partial for JavaScript: Moved JavaScript functionality to a separate
js/addToFavorites.js file
, loaded via a partial in the layout for better maintainability and performance.Local Storage Logic: The local storage checks if an item is already marked as a favorite, and updates the button's appearance accordingly. This avoids the need for server-side calls, keeping it fast and client-side focused.
Screenshots