Skip to content

Conversation

@eliotrhys
Copy link

Changes

  • CTA Component Created
  • Responsive Styling & Grid
  • Hover Scaled Image Effect
  • Accurate Styling From Figma File

Some Considerations

  • Unsure of workflow re: Mono framework usage - better to lift styles from Figma inspector for styling accuracy or to use Mono token mixins to approximate styling?
  • Wasn't sure how granular you wanted the responsive design to be - I used the in-built breakpoint mixins for tablet, desktop and default but wasn't sure if you were looking for more accurate/granular changes. Happy to fix this.

Let me know if you'd like me to make changes ahead of the deadline!

Screenshot 2023-05-21 at 04 59 03

@bbucek
Copy link
Contributor

bbucek commented May 30, 2023

Cards:

  • Pretty good responsive work
  • Follows design nicely

CTA:

  • No need for inline styles
  • Maybe the button should drop below the text on a smaller screen

@eliotrhys
Copy link
Author

Mortified that I left an inline style in there haha - latest commit fixes and adds button wrapping below text as requested.

See commit: [62808e9]

@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. There is not enough spacing between Title and paragraph in CTA.
  2. There is not enough spacing below CTA on mobile and button looks strange.
  3. Cards are squished a bit too much on smaller viewport ~ 770px.
  4. Nice to have: Classes in the CTA component are a bit chaotic. We like using a variation of BEM naming convention, see if you can improve it a bit.

@eliotrhys
Copy link
Author

Changes

  • Fixed spacing between Title and Paragraph
  • Fixed spacing below CTA
  • Added spacing above and below on mobile for more comfortable testing
  • Fixed card grid layout with 1, 2 and 3 span options depending on breakpoints, removed card squish
  • Refactored CTA CSS class naming. More used to simple names within parent component approach, rather than BEM.

I was unsure what you meant about button looking strange, as it's just the main button from the mockup set to block?

If you have a specific change you're looking for on the button, or anything else, let me know and I'll fix it ASAP!

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