Inserted Search Tip Modal Into the Project Page Below the Filter Dropdown

[dvernon5]

Fixes #7866

What changes did you make?

• Inserted a Search Tip Link at the below the filter dropdown in Desktop version and in-between the Filter hamburger in Mobile Version. File path website/_includes/current-projects.html
• The Search Tip Link consists of a modal, so when Search Tip Link is clicked, a modal will appear with content helping the user understand the search bar functionalities better. To check out the implementation, check out the following files: website/_includes/current-projects.html, website/_sass/elements/search-bar.scss, and website/assets/js/current-projects.js

Why did you make the changes (we will use this info to test)?

• To help users understand the boolean operators search bar functionalities.

CodeQL Alerts

After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations.

Check the PR's comments. If present on your PR, the CodeQL alert looks similar as shown

Please let us know that you have checked for CodeQL alerts. Please do not dismiss alerts.

• I have checked this PR for CodeQL alerts and none were found.
• I found CodeQL alert(s), and (select one):
  • I have resolved the CodeQL alert(s) as noted
  • I believe the CodeQL alert(s) is a false positive (Merge Team will evaluate)
  • I have followed the Instructions below, but I am still stuck (Merge Team will evaluate)

Instructions for resolving CodeQL alerts

If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.

In general, CodeQL alerts should be resolved prior to PR reviews and merging

Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)

Visuals before changes are applied

Visuals after changes are applied

[github-actions]

Want to review this pull request? Take a look at this documentation for a step by step guide!

---

From your project repository, check out a new branch and test the changes.

git checkout -b dvernon5-add-search-tips-7866 gh-pages
git pull https://github.com/dvernon5/website.git add-search-tips-7866

[belunatic]

ETA: 3/15/2024 EOD
Avail: 12pm-2pm MWF

[Cloid]

ETA: EOD 3/15
Avail: Weekdays 5pm-9pm

[Six5pAdes]

ETA: 3/14 eod
Avail: weekdays 9am-6pm

[Six5pAdes]

Hi there, @dvernon5! Well done with fixing this issue.

What's Done Well:

• The issue correctly links.
• The PR title is informative, as are both the what and why for its changes in the contents.
• All of the changes made appear correctly on the local environment, both in the code and the resulting updated page.

What Can Be Changed In The Future:

• removing the default images that are shown included in the screenshot section
  image

Feel free to take the suggestion(s) into account for your next issue/PR. In the meantime, I approve this PR.

[dvernon5]

@Six5pAdes

Thank you for your feedback! I took your advice and removed the default images. Great catch. I will be more attentive next time.

[gmgonzal]

ETA: 3/11

[gmgonzal]

ETA: 3/14

[belunatic]

@dvernon5 my brother amazing work here.

few things I run into and if you can correct them:

• on desktop the search bar is on top of the modal, you may want to increase the z-index of overlay. the search bar is z-index:10 and your overlay is z-index: 1
  

• on mobile the modal seems to be off on one side and not centered, this is cause by the overlay width, if you replace width: calc(100vw +17px) with width:100vw should fix it , but try it yourself first.
  

• Do not forget to let us know what files you have added, updated or deleted (🤭) on the "What Changed section"

[siyunfeng]

@dvernon5 , please check if you completed the changes @gmgonzal requested in this comment.
If you do, you can re-request @gmgonzal to review and approve this PR.

[dvernon5]

Hello @siyunfeng

Thank you for reaching out with an update!

I wanted to inform you that I've made the changes according to @gmgonzal request and re-requested his review. Based on the comments, it looks like he will review my issue sometime tomorrow. I understand that everyone has busy lives, so I'm patiently waiting.

[gmgonzal]

• please rename the function to "attachEventListenerTo...". We want the function names to be as close to a description of the functionality as possible!

[gmgonzal]

• please rename the function to "attachEventListenerTo...". We want the function names to be as close to a description of the functionality as possible!

[gmgonzal]

Well done @dvernon5! I see the filter functionality is working again on my setup. Your triage and analysis of the JS issue was very well done, this is what engineering is about :)

I have requested some minor changes to the function names that I think would be good to have. After these changes are made, we should be good to approve!

[dvernon5]

@gmgonzal

Thanks again for your feedback! I do have a different take on the naming convention, though. You mentioned wanting function names "as close to a description of the functionality as possible," and I completely agree, that's why I went with openSearchTipModal and closeSearchTipModal. They're intuitive and directly reflect what the functions achieve, with the event listener logic embedded inside.

Furthermore, I'm concerned that changing them now would mean re-pushing and re-requesting reviews from everyone, which could delay things since the code's already functional. Other reviewers didn't flag this, and I don't see it breaking any established convention. Is this worth the extra round, or can we stick with what's working and approved as-is?

[gmgonzal]

@dvernon5

I see your point. Let me provide more support for this ask. When I first saw the "open/close" function calls, I thought that these functions were effectively opening and closing the modal, which they are not (and this resulted in some confusion for me). What the functions are actually doing is attaching a listener, and only once these listeners are triggered will they "open/close" the modal. I do believe there is a subtle but important difference there.

For example, in file current-projects.js on line 610 you can see how functions that attach listeners have names that reflect that:

    /**
     * Add onclick event handlers to filter tag buttons and a clear all button if filter-tag-button exists in the dom
 */
function attachEventListenerToFilterTags(){

You make a good point about the cost of having another round of reviews. To that I would say that it is not necessary for everyone to re-review since the only change would be to rename a couple of functions that are introduced by your changes and only called once each, so a CodeQL Alert check would suffice :)

Let me know what you think.

[gmgonzal]

@dvernon5

I am seeing the following behavior on web and mobile, could you check if you are seeing the same? It appears like the default table html is always being rendered. Also the Search Tips link is no longer working, the modal is no longer popping up when I click on it.

I am viewing on Firefox.

Website

Mobile

[dvernon5]

@gmgonzal

Woah, Firefox is not doing that on my end. My functionalities remain the same.

Website


Mobile

[dvernon5]

@gmgonzal,

In your URL, try entering localhost:4000. Let's see if that works.

[gmgonzal]

@dvernon5 how strange! After re-branch and re-pulling your changes, I see everything working as expected for both web and mobile. Well done! PR approved :)

[daras-cu]

@dvernon5 thanks for working through all the changes on this, everything looks good when I tested the modal. I do agree with @gmgonzal about renaming the two functions that add event listeners to open/close the modal so they're more clear. You don't have to wait for everyone to re-review afterwards, I will merge it as soon as you make those changes.

I also think some of the other points @gmgonzal made suggest we may want to open an ER to investigate making the modal a reusable element since we now have them on two pages. I know the functionality is slightly different on the Wins page since the modal is toggled off on mobile, but the CSS and Javascript could potentially be factored out into their own files. Let me know your thoughts knowing how the modals on both pages work.

[dvernon5]

Hello @daras-cu

Thank you for your feedback. I renamed the two functions to make the codebase clearer.

Regarding the modal, I think you're right we could make the modal reusable by factoring out the CSS and JS into separate files. Since the modal's style and structure are the same on both the Wins and Projects pages, we could create a single modal.css file for the shared styling and a modal.js file for the core functionality. The main difference is the mobile behavior, it could be handled in the JS with a condition like if (window.location.pathname === '/winsPage') to toggle a dropdown instead of the modal on mobile while keeping the modal behavior for /projectPage.

I believe this approach could work since the Wins page modal is toggled off on mobile anyway, and. a dropdown could replace it there without affecting the Projects page. We'd just need to make sure the JS is flexible enough to detect the page and plan accordingly. What do you think about this strategy?

I hope I answered your question correctly.

[daras-cu]

Thanks for making the requested changes @dvernon5, approved. @gmgonzal I think we will address your remaining comments in an ER to create future issues to factor out the modal code to be reusable.

[gmgonzal]

Awesome, thanks @daras-cu and @dvernon5 !