Fixed the CSS of Eli Selkin card in Engage's Project page was: displayed nonuniformly

[codyyjxn]

Fixes #7429

What changes did you make?

• I resolved the issue by adjusting the @media #{$bp-below-mobile} to flex-direction: row; , ensuring it didn't impact the landscape view or responsiveness across mobile and web platforms. I also verified that the changes-maintained user-friendliness and dynamic functionality throughout.
• Also added flex: 1 to the description for each card. This gave a seamless interaction for the user.

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

• The reason I made these changes in engage project page, was because the text for Eli Selkin's card did not
  have the same visual margin as the other cards when displayed in phone portrait view.
  • I addressed this issue to ensure it aligns with the user-friendly standards we uphold at HackForLA

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 codyyjxn-update-textCard-7429 gh-pages
git pull https://github.com/codyyjxn/website.git update-textCard-7429

[tamara-snyder]

Review ETA: End of day 9/22/2024
Availability: Morning and evening

[tamara-snyder]

Hi Jonathan, I think the padding solution you've come up with works well. I noticed the same issue was happening in the following places, and it looks like your fix has resolved the issues in those spots as well 😄

• 311 Data:
  • Anna Kim
• Heart:
  • Charlotte Soestini
• Expunge Assist:
  • Maria Weissman
• LA TDM Calculator:
  • Bonnie Wolfe
• Tech Work Experience:
  • Joshua Fishman
• EMS Triage Tracker:
  • Shawn Duenas
  • Simone Wojtaszek
  • Nicole Doan
  • Conor McKiernan
  • Selena Jiang
  • Kelvin Nguyen
• Civic Tech Index:
  • Bonnie Wolfe
  • Bhaggyalakshmi Balasubramaniyan
  • Maxwell Countryman Skewes

I did notice that a bit of text is spilling off the cards at around 487x1183. Is it feasible/in scope to resolve this within this issue, perhaps by moving the text to the left a bit?

Otherwise, looks great! Really nice work on this!

[santisecco]

Review ETA: 6 PM 9/23/2024
Availability: 1-3 PM Monday-Tuesday

[daras-cu]

@codyyjxn as it appears the display issue is only occurring at mobile size, you really only need the increased padding under the mobile breakpoint (@media #{$bp-below-mobile}). This should resolve the issue @tamara-snyder noted with the text being off to the right/overflowing at larger screen sizes.

[codyyjxn]

@tamara-snyder @daras-cu Thank you for the feedback I went ahead and implemented it seems to work better. Thanks again! Can you please review Thanks !

[santisecco]

Hi @codyyjxn great job! Together with the last changes, suggested by @daras-cu, the view improves considerably.
Some text, similar to what @tamara-snyder noted, still spills off around 484×5363.
However, I don't know if that can be further improved.
Other than that, the code is correct, as well as your explanation of your changes.
Thanks for working on this issue.

[codyyjxn]

Hi @codyyjxn great job! Together with the last changes, suggested by @daras-cu, the view improves considerably. Some text, similar to what @tamara-snyder noted, still spills off around 484×5363. However, I don't know if that can be further improved. Other than that, the code is correct, as well as your explanation of your changes. Thanks for working on this issue.

@santisecco Oh! Could you please share a screenshot of what you're seeing on your end, particularly where the issue is still occurring? I’d like to take a look and see how I can improve it, if necessary.

[santisecco]

Hi @codyyjxn I couldn't replicate the problem still, now that I think about it, I believe the dimension I mentioned 484x5363 is not usual. Honestly my exp in front end is limited. So anyways, using other dimensions there are still some issues I hadn't seen before:

Though here's one that looks good 👍

[santisecco]

Ok just to give an update @tamara-snyder @codyyjxn @8alpreet @daras-cu. We'll be following @8alpreet solution. So the last PR version is the one to check.
It was decided with Bonnie @tamara-snyder and the rest of devs in today's meeting.

[codyyjxn]

Ok just to give an update @tamara-snyder @codyyjxn @8alpreet @daras-cu. We'll be following @8alpreet solution. So the last PR version is the one to check. It was decided with Bonnie @tamara-snyder and the rest of devs in today's meeting.

Thank you all for the update I wasn't able to attend the meeting today but I really got to understand the UI a lot better. Thanks all for the input. Is there anything I need to improve before this gets merged ? @tamara-snyder @santisecco @8alpreet @daras-cu

[8alpreet]

@codyyjxn, thanks for the update! I will complete my review 7pm today.

[8alpreet]

Hi @codyyjxn,

Great job on this issue, I know it became complex and took longer than we expected. You stuck through it, so props for that! I locally tested the changes across different projects and the cards look great.

Things you did well:

• PR title is descriptive
• Issue is Linked
• Branch name includes issue number
• What section describes the changes
• Before and after pictures clearly outline the changes

Room for Improvement:

• OPINION - The PR title can be updated since the current one, though accurate to the issue, does not accurately reflect the changes. So something like "Updated CSS to fix text alignment issue in project leader card" could work.
• If you want the keep the current title, the "--7429" portion is definitely not necessary.
• OPINION - The why section could use more specific details; although you answer the question, it is high level and a bit vague. A quick insight into why these specific changes were needed would be wonderful.

Notes:

• the CNAME file should be not changed but looks like that issue was already discussed and resolved.

[codyyjxn]

@tamara-snyder @santisecco @daras-cu Please review when possible. Thank you !

[daras-cu]

Thanks for continuing to work on this @codyyjxn, I think having the text remain next to the picture for smaller screen sizes is nicer visually and more consistent with how the page displays at larger sizes. I know it went a little beyond the scope of the original issue, but I'm glad you got to learn more about the UI and page styling in the process.

Everything looks good when I test your branch locally and your PR is set up correctly. Well done!

[santisecco]

Hi @codyyjxn thanks for working on this issue. As discussed in the meeting the last layout is the one to be adopted and it's looking great. Also the approach chosen mentioned by @8alpreet simplifies the code in my opinion.

I would like to also thank the code suggestions and help of @daras-cu, @8alpreet and @tamara-snyder. They for sure contributed a big deal to solving this.

Great job

Dismissing this because we're going with Balpreet's solution

[tamara-snyder]

Hey @codyyjxn, great work on this and thanks for all your patience trying out various changes! I think you've executed this approach well. Thanks also to @8alpreet, @santisecco, and @daras-cu for sharing your thoughts and solutions - I learned some useful tips from this discussion.

[jphamtv]

Hey @codyyjxn, great work collaborating with everyone on this PR to come up with an elegant solution.

This is ready to be merged, but first, can you please fix the CNAME file? I noticed there's an extra blank line (line 2) introduced, which should be removed. Having extra blank lines in a CNAME file may cause issues with DNS resolution, potentially affecting the domain configuration.

Thanks!

[codyyjxn]

Hey @codyyjxn, great work collaborating with everyone on this PR to come up with an elegant solution.

This is ready to be merged, but first, can you please fix the CNAME file? I noticed there's an extra blank line (line 2) introduced, which should be removed. Having extra blank lines in a CNAME file may cause issues with DNS resolution, potentially affecting the domain configuration.

Thanks!

Hello @jphamtv . I tried to uncommit the CNAME file but for some reason I can't seem to get rid of that change. What would you recommend I do ?

[jphamtv]

Hello @jphamtv . I tried to uncommit the CNAME file but for some reason I can't seem to get rid of that change. What would you recommend I do ?

@codyyjxn - Thanks for trying to resolve this. Since uncommitting didn't work, let's do it manually:

1. Open the CNAME file in your editor
2. Delete the blank line (line 2)
3. Commit and push the changes

[codyyjxn]

Hello @jphamtv . I tried to uncommit the CNAME file but for some reason I can't seem to get rid of that change. What would you recommend I do ?

@codyyjxn - Thanks for trying to resolve this. Since uncommitting didn't work, let's do it manually:

1. Open the CNAME file in your editor
2. Delete the blank line (line 2)
3. Commit and push the changes

Thanks ! This worked I was able to remove that file from this pr. @jphamtv
Do I need to request another review for this pr?

[jphamtv]

Thanks for the quick update @codyyjxn, approved and merged.