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

Issue 1293: Add a card for mobile #1295

Merged
merged 4 commits into from
Dec 26, 2024

Conversation

jasonwang7517
Copy link
Contributor

@jasonwang7517 jasonwang7517 commented Dec 21, 2024

🔗 Issue

Original issue

✍️ Description

Adds responsive sizing for the fosters tab for smaller displays.

📷 Screenshots/Demos

Gradual resizing
ScreenRecording2024-12-26at12 16 46PM-ezgif com-video-to-gif-converter

Quick mobile view
https://github.com/user-attachments/assets/9435a2dc-66c1-42bf-a034-1f595c1a155e

@jasonwang7517
Copy link
Contributor Author

Side note: I tried testing what this would look like with multiple foster records and I ran into some database confusion.

I created another instance of Match with the same pet_id and organization_id but a different person_id and when I ran .matches.fosters on the Pet with the same id, everything turned up fine.

However, when I tried to render it never showed up. When I printed the number of matches in the view file upon rendering, it only ever showed the original match, even after manually calling @pet.reload in the view as well.

Not sure if I'm missing something there but that's why you only see one card in the demo.

@jmilljr24
Copy link
Collaborator

@jasonwang7517 I'd have to look at the logic but it sounds like you tried to create it via the console? You should be able to create a new forster at /alta/staff/manage_fosters. If that doesn't work please let us know. This what two look like for me:

Screenshot from 2024-12-21 08-28-27

This is the applications tab referenced in the issue. Can you try to do something similar? There might be a partial in alta/staff/manage_fosters that you could probably use.
Screenshot from 2024-12-21 08-40-55

Copy link
Collaborator

@kasugaijin kasugaijin left a comment

Choose a reason for hiding this comment

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

Oh nice you did this without having separate markup for mobile that is hidden. This is better for sure. This is what you were referring to @jmilljr24 ?

@@ -1,24 +1,24 @@
<div id="fosters_tab_table" class="card">
<div id="fosters_tab_table" class="card col-6 col-xl-12">
Copy link
Collaborator

@kasugaijin kasugaijin Dec 22, 2024

Choose a reason for hiding this comment

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

So this will be a full width table above 1200px right? Then below this it will collapse into half width and flex will place items accordingly (referring to the use of flex-xl-row below).

I am curious how this looks as you drag screen width from 1200+px down. I imagine that we will show the row above 1200px then show 'cards' below 1200px. In my mind, there's a lot of possible devices (tablets or horizontal cell) that might do fine with a table row below 1200px? So XL might be a bit of a large breakpoint? Do you think we can set this breakpoint at Lg or even Md?

@jmilljr24
Copy link
Collaborator

Oh nice you did this without having separate markup for mobile that is hidden. This is better for sure. This is what you were referring to @jmilljr24 ?

@kasugaijin Yes, however I'm not sure this hits the mark are far as ui/ux goes. I think for the time being I would use the design we already have using separate markup for desktop and mobile. This keeps a consistent look and feel until we think more about a design for the person profile page. If we can get something the checks the box for a single markup response design on the person profile page, we could then circle back to the pets page.

@kasugaijin
Copy link
Collaborator

Oh nice you did this without having separate markup for mobile that is hidden. This is better for sure. This is what you were referring to @jmilljr24 ?

@kasugaijin Yes, however I'm not sure this hits the mark are far as ui/ux goes. I think for the time being I would use the design we already have using separate markup for desktop and mobile. This keeps a consistent look and feel until we think more about a design for the person profile page. If we can get something the checks the box for a single markup response design on the person profile page, we could then circle back to the pets page.

Yeah I have to agree with this. @jasonwang7517 could you please see how we implement this for other pages. We actually render two sets of 'duplicate' markup, and hide one depending on the screen width. In this case, you need to add the markup that will show on smaller screens. See existing implementations for breakpoints and card layouts.

@jasonwang7517
Copy link
Contributor Author

Oh nice you did this without having separate markup for mobile that is hidden. This is better for sure. This is what you were referring to @jmilljr24 ?

@kasugaijin Yes, however I'm not sure this hits the mark are far as ui/ux goes. I think for the time being I would use the design we already have using separate markup for desktop and mobile. This keeps a consistent look and feel until we think more about a design for the person profile page. If we can get something the checks the box for a single markup response design on the person profile page, we could then circle back to the pets page.

Yeah I have to agree with this. @jasonwang7517 could you please see how we implement this for other pages. We actually render two sets of 'duplicate' markup, and hide one depending on the screen width. In this case, you need to add the markup that will show on smaller screens. See existing implementations for breakpoints and card layouts.

Yes for sure! So sorry for not being able to get to this this weekend, will have more time this week.

@kasugaijin
Copy link
Collaborator

@jasonwang7517 no rush!

@jasonwang7517
Copy link
Contributor Author

jasonwang7517 commented Dec 23, 2024

Okay folks! I duplicated and slightly refactored the existing partial pattern and it looks much better! A little bit of non-DRY stuff but I think this helps. We could explore abstracting these partials even more but given the difference in data (pet name, image, etc.) that may be more work than it's worth.

</ul>
</div>
</div>
<% end %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

new line :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oop my bad I'll add it back!

Copy link
Collaborator

@kasugaijin kasugaijin left a comment

Choose a reason for hiding this comment

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

@jasonwang7517 could you please provide a quick gif/video of the transition? I am sure it works fine, but it helps with review.

Copy link
Collaborator

@kasugaijin kasugaijin left a comment

Choose a reason for hiding this comment

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

You need to merge main branch again to get rid of these changes. (Make sure you you have pulled the latest main).

Copy link
Collaborator

@kasugaijin kasugaijin left a comment

Choose a reason for hiding this comment

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

@jasonwang7517 thank you!

@kasugaijin kasugaijin merged commit a765c34 into rubyforgood:main Dec 26, 2024
5 checks passed
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.

3 participants