Skip to content
This repository has been archived by the owner on Jan 27, 2022. It is now read-only.

fixed image height in card #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shahednasser
Copy link

Fixes issue #3

Note: After trying to contribute I have a few notes that can help other contributors contribute to this repository:

  1. Add in the README that npm install should also be run in client as it is not clear.
  2. For some reason I was not able to run npm run dev I kept getting an error where axios was not recognized. Maybe it should also be added to the package.json in root but I haven't tested it.
  3. I recommend adding a seed script where dummy data can be added to the database. This will make it easier for contributors to make their contribution without getting into the hassle of adding data themselves. You can use something like faker.js to generate fake data easily!

Keep up the good work and thank you for allowing me to contribute!

@itsnitinr
Copy link
Owner

Thank you for your valuable suggestion and contributions. I have hopefully fixed the second issue as you mentioned. It was caused because axios wasn't in package-lock.json. However, it was in yarn.lock as I installed it using yarn. I think you used npm but I use yarn primarily hence I didn't face the issue. Thank you for bringing it to light. I will remove one of the two lock files as it may create confusion. Will merge your PR soon. Once again, thank you for everything.

@shahednasser
Copy link
Author

@itsnitinr that makes sense. I suggest adding that it's more preferable to use yarn for this project to instruct contributors about it!

@itsnitinr
Copy link
Owner

itsnitinr commented Sep 30, 2020

@shahednasser Hello, there. By setting max-height to 100%, there is a chance of getting layout issues in case a user uploads a mobile screenshot.

After your changes:
Screenshot_20200930_171020

Before changes:
Screenshot_20200930_171120

@shahednasser
Copy link
Author

I noticed that actually but I found that this issue was present at some screen resolutions. This is due to the fact that the images have different sizes. Maybe another fix would be to set the width of the card based on the image? That way the image wouldn't be smaller than the card itself.

@itsnitinr
Copy link
Owner

Yup, correct. I will try to look into it. Please don't mind if the PR stays open for a while.

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

Successfully merging this pull request may close these issues.

2 participants