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

Converted save image button #378

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

Conversation

ggsawatyanon
Copy link
Contributor

Summary

This pull request fixes the bug where our save apartment button is an image and not an actual button as requested by PM.

  • Converted save apartment image button to normal button component

Test Plan

image Apartment when saved: image

Notes

  • Currently, on mobile there is no way to actually save the apartment. I tried playing around with different locations to put a bookmark icon button but the space is quite cramped and the different versions I tried out made the UI too cluttered or too small, I'm going to ask designers about their opinions on where we can add the save button on mobile.

@dti-github-bot
Copy link
Member

dti-github-bot commented Nov 17, 2024

[diff-counting] Significant lines: 52.

Copy link
Contributor

@CasperL1218 CasperL1218 left a comment

Choose a reason for hiding this comment

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

This is a simple and great fix to the historical issue; the implementation is clean and works well!

Screen.Recording.2024-11-17.at.23.31.38.mov

One small frontend enhancement: Currently the hover state of the saved button moves many components of the page as shown. It might be better to remove this effect unless this is an intentional design.

@kea-roy
Copy link
Member

kea-roy commented Nov 27, 2024

PR Changes

  1. Resolved border resize issue when hovering.

Copy link
Member

@kea-roy kea-roy left a comment

Choose a reason for hiding this comment

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

This PR is great and the button's design works as intended. After resolving minor issues with material-ui styling, the button now works perfectly.

@kea-roy kea-roy dismissed CasperL1218’s stale review November 27, 2024 20:46

Resolved resizing issue

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.

4 participants