hackforla / website

Hack for LA's website
https://www.hackforla.org
GNU General Public License v2.0
286 stars 700 forks source link

Fix `Our Events` Card Glitch After Toggling and Resizing Window #6777

Closed tony1ee closed 2 weeks ago

tony1ee commented 2 weeks ago

Fixes #6775

What changes did you make?

  1. (7e94b92) changed where eventListener is registered from .flex-page-card' to window
  2. (4262f25) added a condition before handleScreenResize() collapse cards to check if they have been manually unfolded, and if so, leave them unfolded

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

Screen Recordings of Proposed Changes Of The Website

Visuals before change 1 is applied notice how the card will be empty after resizing from width 750 to width 800 ![b1](https://github.com/hackforla/website/assets/16524851/d4e4982f-fc03-4d49-95f4-a4140763ee72)
Visuals after change 1 is applied notice how the card content **remains** after resizing from width 750 to width 800 ![a1](https://github.com/hackforla/website/assets/16524851/b8348efa-c010-4028-a93e-e6c5eeb29cf0)
Visuals after change 1 is applied but before change 2 is applied notice the card collapse unexpectedly while adjusting the width (<=767 the entire duration) ![b2](https://github.com/hackforla/website/assets/16524851/2bfe6742-5240-4b91-bb4b-fb5fd30edaef)
Visuals after both change 1 and change 2 are applied expected behavior: the unfolded card remains unfolded while adjusting the width (<=767 the entire duration) ![a2](https://github.com/hackforla/website/assets/16524851/407fefe8-b99b-4cc7-b3cf-ae20ca78e4e8)
gaylem commented 2 weeks ago

Note for other reviewers: The Add Pull Request Instructions check is failing because of #6778. The changes were reverted, but its still not working for this issue. Here's how I set up my branch locally so you don't have to figure it out:

git checkout -b tony1ee-fix-events-card-glitch-6775
git pull https://github.com/tony1ee/website.git fix-events-card-glitch-6775

Note for merge team: I'm guessing we can ignore that failed check and just squash and merge when this gets two approvals. Let me know if you disagree.

tony1ee commented 2 weeks ago

@gaylem Thanks for your thorough review.

You were correct to point out the behavior of the card when first switched to/loaded mobile view. I am not sure what is the expected behavior in terms of default card display. I would consider it out of scope of this issue and suggest doing an issue involving UI/UX to address it further.

As for your 2nd concern, I pushed another commit with comments and some format adjustments. I tried my best to balance between maintainability and keeping the changes within issue scope. Please let me know your thoughts!