Skip to content
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

Feat: 1091 present fostered start and end date on fostered pets index page #1217

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

odellmac4
Copy link

@odellmac4 odellmac4 commented Dec 8, 2024

🔗 Issue (#1091 (comment))

✍️ Description

This PR adds a "Term" section under each fostered pet's name, displaying the start and end dates of the foster term. To fit the "Term" and dates within the card while maintaining a responsive design, I slightly adjusted the width of the card. The layout is now more mobile-friendly and should work well across most screen sizes.

Changes Made:
Start and End Date Added:
The start_date and end_date for each fostered pet are now displayed on the card, formatted as MM/DD/YY.

Card Layout Adjustment:
I extended the card’s width to accommodate the additional date information and make it more mobile-friendly. This change ensures that the start and end dates fit within the available space and don’t overflow or cause layout issues on different screen sizes.

Date Format:
I decided to abbreviate the date format to MM/DD/YY because the full month name made the card too wide, and I had to extend the card width significantly to fit it. However, I want to confirm if this abbreviated format is acceptable or if you'd prefer the full month written out (e.g., December 08, 2024). Please let me know your preference.

Testing for Views:
I noticed that there don't seem to be any tests for views in this repository. Could you please clarify whether we test views? If we do, I'd love to know where the relevant tests are located or if view tests are not part of the testing strategy in this repo.

Viewport Size (800 x 1236):
I tested the responsiveness of the layout with various screen sizes, and I encountered an issue with the screen size 800 x 1236. The layout is not optimized for this particular size, but I understand it's not a common screen size. I’d appreciate your guidance on whether we need to optimize the UI for this viewport size or if it’s something that can be excluded from our responsiveness considerations.

Screenshot 2024-12-07 at 10 26 28 PM

Summary of Code Changes:
Start and End Dates added for Fostered Pets
Card layout adjusted to fit dates and be mobile-friendly.
Date format changed to MM/DD/YY (pending confirmation).
Thank you for your review and feedback! Let me know if there are any changes you’d like me to make.

📷 Screenshots/Demos

Screenshot 2024-12-07 at 10 26 07 PM Screenshot 2024-12-07 at 10 26 47 PM

@odellmac4
Copy link
Author

Please lmk your thoughts I refactored it to reduce the amount of text on the card since the card is quite small. I will add a screenshot of what it initially looked like and am happy to change it back if preferred.

Screenshot 2024-12-07 at 9 59 34 PM

<h5 class="card-title mb-0 flex-grow-1" style="margin-right: 15px;"><%= fostered_pet.pet.name %></h5>
<div class="fostered-dates ms-auto">
<div class="d-flex flex-column">
<h5 class="card-title mb-1">Term:</h5>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually prefer what you showed where you had Start date: and End date: as I think it looked better and was more descriptive!

@@ -13,7 +13,13 @@
class: 'rounded-circle',
style: "width: 100%; height: 100%; object-fit: cover;") %>
</div>
<h5 class="card-title mb-0"><%= fostered_pet.pet.name %></h5>
<h5 class="card-title mb-0 flex-grow-1" style="margin-right: 15px;"><%= fostered_pet.pet.name %></h5>
Copy link
Collaborator

@kasugaijin kasugaijin Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you just use flex on this you should be able to use flex classes like justify-content-around to space these main elements evenly in the parent div https://getbootstrap.com/docs/4.0/utilities/flex/#justify-content

You should not need to use inline style here like style="margin-right: 15px;" as Bootstrap has classes for this.

It's a smell if you need to use margin right on the left hand element, AND ms-auto on the right hand element. If we're using flex, there are classes built for this.

@kasugaijin
Copy link
Collaborator

@odellmac4 how's this going?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants