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

Update Services CTA pattern with tokens #205

Closed
wants to merge 5 commits into from
Closed

Update Services CTA pattern with tokens #205

wants to merge 5 commits into from

Conversation

colorful-tones
Copy link
Member

@colorful-tones colorful-tones commented Sep 5, 2023

Description

Addresses #164

Figma Component link

These pattern updates utilize the recently introduced spacing presets.

Clarifying deviations from design:

  • There is no mobile design and it is hard to know what or where any side margins may exist, but it currently looks undesirable (see small viewport screenshot below)
  • The image in the pattern (tt4_services_1.png) had a fixed width of 679px. Therefore, if the screen size is large then it will not fill the entire column and is currently left-aligned. I re-exported the image as a .jpg and ran through Compressor.io. Now it fits the column better for larger screens.

Screenshot 2023-09-05 at 3 40 48 PM
Screenshot 2023-09-05 at 3 40 59 PM
Screenshot 2023-09-05 at 3 41 08 PM
Screenshot 2023-09-05 at 3 41 18 PM

Testing Instructions

  1. Activate Twenty Twenty-Four theme
  2. Create a page/post and add the 'Services - Call to Action' pattern, and Save
  3. Preview results on published page/post

@richtabor
Copy link
Member

Here's how I did it: #204

"variations": {
"rounded": {
"border": {
"radius": "16px"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this change is interesting to consider for a future refactor of the patterns @richtabor

Copy link
Member

Choose a reason for hiding this comment

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

Yea I'm a bit torn. I kinda think the default radius should be what all the images in the theme are; so you don't have to apply it to others. But I think it could go either way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a few steps to revert the change when you don't need it. We have a pattern with logos that looks bad if we do this and forget to undo the radius

Copy link
Member

Choose a reason for hiding this comment

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

It's a few steps to revert the change when you don't need it. We have a pattern with logos that looks bad if we do this and forget to undo the radius

It could be done at the pattern level though; to remove border-radius. Instead of having to apply border radius for every image added. @colorful-tones do you mind moving the variations.rounded.border.radius change to a separate PR for testing. I'd love to try it out and see if that works (to then add rounded variation to most images within the patterns).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm just working off of Figma designs is all.
Screenshot 2023-09-05 at 4 50 50 PM

Copy link
Member

Choose a reason for hiding this comment

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

Right, but we apply a deeper level of integration/consistency with the block editor :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@colorful-tones do you mind moving the variations.rounded.border.radius change to a separate PR for testing.

I'd love to try it out and see if that works (to then add rounded variation to most images within the patterns).

Oh it works @richtabor . Did we just think I dropped it in for fun? 🙃

@colorful-tones
Copy link
Member Author

It is funny how we were both working on the same thing at the same time.

Here is an interesting comparison of two different variations of interpreting provided Figma designs from seasoned builders. 😆

@richtabor @MaggieCabrera

tt4 local_services-cta_

@richtabor
Copy link
Member

richtabor commented Sep 5, 2023

Closing as it’s been tidied up in #204

@richtabor richtabor closed this Sep 5, 2023
@colorful-tones colorful-tones deleted the feature/service-cta-tokenize branch September 5, 2023 22:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants