add search functionality #3174
add search functionality #3174
Conversation
|
Aloha @dragon-slayer27 🙌 - Thanks for your contribution! FeedbackTip You can refer to README.md for additional guidance.
Happy Coding! 🚀 |
The intention is not to add a CSS animation, but to implement a search bar, so those changes are required. |
LaurelineP
added
Out of scope
There was a problem hiding this comment.
Aloha @dragon-slayer27 🙌
Thanks for the interesting contribution!
Please note that your current contribution is out-of-scope based on what the README.md suggests. I will double check and discuss your PR with other maintainers members.
Meanwhile I took the time to review your entire contribution and provided feedbacks you could address ( either by file, by line(s)) and the following ones concerning the feature and overall proposition you are proposing.
Feature and implementation completion feedback
-
Showcasing x artworkstext in the HTML:
This displays the numbers of artworks but when adding a search feature it would be insightful to know how many it has been found.
I would suggest the following text:Showcasing x artworks | y found
-
Clearable input - with a clear button
For UX reasons, it is best to have a "clear" button within the input that allows to
delete all characters at once if the user choose to - currently we have to delete
manually all the characters to get back to the full list
-
Cards width & responsiveness - to address
The changes modified visually how cards are (individually wider ) and behave - impacts on how many cards are displayed
UX issue with search bar and navigating back
Reproduction:
- search: search for x artname or author
- art selection: click on one artwork to discover ( we are redirected to the artwork animation page )
- going back action: click on "go back" button of your browser to go back to the list
Observations:
Correct element: search field remains with the searched value
Incorrect elements:
- the listed items does not correspond to the searched value ( the refacto of all the changes done in includes.js should help as you would only have to trigger the function containing your search logic.
Keep the input value as it is ( with the searched value ), this is actually great for UX to keep a consistent experience to the user navigation
- On going back to the page after viewing an animation, the list of x found results should be the same / ideally the same order
Note: This behaviour is also partly because you trigger the searching process only on "typing" a value
|
Thanks for the feedback. I will inform you as soon as I have completed implementing the suggestions. |
|
Aloha bis @dragon-slayer27 |
|
ok thanks for informing @LaurelineP ❤️ |
|
Aloha @dragon-slayer27 🙌 - Thanks for your contribution! FeedbackTip You can refer to README.md for additional guidance.
Happy Coding! 🚀 |
…r27/Animation-Nation into feat/add-search-bar
|
Aloha @dragon-slayer27 🙌 - Thanks for your contribution! FeedbackTip You can refer to README.md for additional guidance.
Happy Coding! 🚀 |
|
Aloha again @dragon-slayer27 🙌 For you to have this on your PR,
Enjoy 🔥 |
|
@LaurelineP, thank you for providing the in-depth and precise review. I have completed all things from my side. |
There was a problem hiding this comment.
Aloha bis @dragon-slayer27 !
First of all, well done on keeping your PR up to date!
You’re showing great ownership of your work — which is an essential skill when collaborating in a team.
Also, excellent job addressing such a large amount of feedback! 🎉
You’re really grasping how collaboration and the PR workflow function in practice.
Regarding the PR acceptance I am still awaiting for news. I will let you know as soon as a I have an answer but for now the discussion is still in progress.
Overall review
You’ve addressed all previous points, and I don’t see any regressions.
You’ve clearly put in the effort to iterate on your code, great progress! 🙌
You are definitely getting there !
Here are few more pieces of feedback (these should be much lighter than the first round, which involved a larger refactor)
✨ Additional Feedback
🔹 Comments
Tip
Pro tips : So far, you've used regular comments both to describe what your code or functions do.
There are different types of comments, each with their own purpose.
// ...: *Inline comments – great for short explanations and quick notes/* ... */: *Multiple lines or block comments - great for bigger explanation/** ... */: ➡️ Documentation comments (JSDoc) - parsed by IDEs and displayed when hovering over a function name.
Many developers - even experiences ones - tend to forget about documentation comments
which should be the comment placed before the function definition.Once you try it, you’ll see how useful it is: your IDE will show the function’s description
when you hover over its name.I invite you to :
- Experiment with documentation comments to see how they appears in your IDE
- Which leads us to this task: review the comments of the function to use documentation comments instead .
🔹 PR workflow and interactions.
I’ve noticed that you’ve been resolving conversations and addressing feedback step by step
while updating your code — that’s great to see! 👏Here’s another pro tip for you:
Usually, the author of the comment (the reviewer) should be the one to mark a conversation as resolved.t so when you get into a team you can either ask about their workflow and what they decided to do.
That said, don’t worry — even many experienced developers aren’t always sure how to handle this part of the PR process.
It’s a great thing to be aware of, though, because once you start working in a team, you’ll know to either ask about their preferred workflow or follow what’s already been established – you could even be the one suggesting the approach.Extra tip - good to know :
In more advanced repositories, unresolved feedback can actually block a PR from being merged – which isn’t the case here.If you ever work in a team where practices around this aren’t consistent, showing strong ownership of your PR means waiting for the reviewer to confirm and resolve the conversations after you’ve pushed your changes.
If you’ve already requested their review and notice they haven’t resolved the discussions but you need to merge, that’s when it’s fine to resolve them yourself.
At least that way, you’ll have followed the best practices for PR collaboration.🤓 Few resources
Once again congratulations, I hope you are enjoying your experience 🙌
Happy coding and hacking !
public/includes.js
Outdated
| function renderMessage(container, message) { | ||
| if (!container) return; | ||
| container.innerHTML = `<p class="no-results">${message}</p>`; | ||
| } |
There was a problem hiding this comment.
Function naming
Nice to see the refactored function 👌.
The current function name, renderMessage feels quite generic.
As it stands, it could be mistaken for an utility that renders any "type" of message.
However the function has a very specific purpose: rendering a styled message with "no-results" class (statically applied)
To make the intent cleare for others reading or reusing the function, consider renaming it
- Rename the function to clarify its use (E.g.:
renderNoResults,renderNoResultsMessage)
public/includes.js
Outdated
| if (query === "") { | ||
| statsElement.innerHTML = `Showcasing ${masterCardList.length} artworks`; | ||
| renderCards(cardsContainer, shuffle(masterCardList)); | ||
| } else { | ||
| const filteredList = masterCardList.filter((card) => { | ||
| if (!card) return false; | ||
| const artName = (card.artName || "").toLowerCase(); | ||
| const author = (card.author || "").toLowerCase(); | ||
| return artName.includes(query) || author.includes(query); | ||
| }); | ||
| statsElement.innerHTML = `Showcasing ${masterCardList.length} artworks | ${filteredList.length} found`; |
There was a problem hiding this comment.
StatsElement similar action
Context: l66 and l76.
Like the renderMessage code you've address, these two lines are sharing a same purpose :
rendering the stats.
Improvements
You could definitely make a function out of is instead of repeating how the message should display
I would suggest a function that accept two parameters:
- the length of the card list
- an optional length for the filtered card list
Then the logic would be if there is the second param render the extra bit of states ( found result from the search : "| found" )
Todos
- Extract the repeated logic to a function :)
|
@LaurelineP i really liked learning about JSDoc comments. I had always seen these kind of descriptions on functions but never given any to my own functions. |
|
Alohaaaa @dragon-slayer27 🙌 |
2 participants
While you're waiting, please look over this checklist. Feel free to write a description of what you did as well!
What I did
I have added functionality to search through the artworks by either Art Name or Author. To achieve this, I have refactored the public/includes.js script to follow a more modern and robust "re-render" approach.
Live Search Bar: Implemented a persistent search bar that filters the artworks in real-time as the user types. The search bar is sticky and remains available as you scroll, consistent with the main header.
JavaScript Refactor: The includes.js file was completely overhauled to be a single, cohesive script. This new approach was taken because adding search functionality was difficult with the previously cluttered code. I have not gotten rid of any functionalities that were there previously, only rewritten them.
Pull request checklist
meta.jsonfile.<github_username>-<art_name>without the <>.index.htmlandstyles.css.index.htmlstyles.cssmeta.jsonwith:githubHandle: being your unique github user name,artName: name of your animationNot normally needed
Note: if your pull request makes an improvement besides adding a CSS animation you will need to discuss this with the maintainers. You can help kickstart this process by explaining what your change does and why you are making it in the "What I did" section