Skip to content

Conversation

@TiffanyBabey
Copy link

That's my attempt at the task! Thank you for taking the time to check it out, hope it's what you expected! I had a lot of fun doing it and appreciate the experience whatever the outcome.
Tiffany

@bbucek
Copy link
Contributor

bbucek commented May 30, 2023

Cards:

  • Smallest screens could be better, but the overall look of the responsive is good
  • If cards were flexible width instead of fixed units could be better
  • Too much absolute positioning

CTA:

  • Responsive alignment could be better.

@rafzielinski
Copy link
Collaborator

Hi and thank you for your submission.
We like your implementation but we want to ask for few tweaks before we get to the final decision.

  1. CTA button should use an icon not the text chevron, it is also not aligned correctly on smaller screens and missing spacing
  2. CTA missing spacing between itself and the listing component.
  3. CTA body has set width which is not responsive approach.
  4. There is no need for any position: absolute in this project, try to fix it.
  5. Cards have set width which is not really responsive.
  6. Card hover effect is not happening when hovering over the text on card.

@TiffanyBabey
Copy link
Author

TiffanyBabey commented Jun 2, 2023 via email

@TiffanyBabey
Copy link
Author

Hi!

The above commits have my implementation of the feedback you share with me.
It looks like the original repo has changed since I forked it so there are conflicting files; please let me know if I should attempt to resolve the conflicts on my side and create a new PR!

Thank you for your consideration.

@TiffanyBabey
Copy link
Author

Hello again,

I accidentally altered my version of the task by trying to resolve the branch conflicts when I submitted my revised version earlier this week, and only noticed just now. The above commit and force push correspond to me fixing this. I hope this is okay!

Many thanks

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