THEMES-1195 Updated headline, overline and image opacity #1690

malavikakoppula · 2023-06-29T14:14:07Z

Description

Review doc: https://paper.dropbox.com/doc/Themes-hover-states-audit--B400R8DxsjiVy0YImyMBZ0B4Ag-JyHvxHuZttlIfj5BzjTI3

Redesigned button components: https://www.figma.com/file/vWVGgZfZVBdSX7ACo3kmjr/Enhanced-Styling-Design-System?type=design&node-id=7736%3A104212&t=wkfbbYPc5dtqGGKM-1 - Connect your Figma account

Jira Ticket

THEMES-1195

Acceptance Criteria

Applies to all promos (manual and non, S-XL):

image when link:

a. opacity change to 80%
overline (only on XL, L):

a. rest: $text-color

b. hover: $text-color-subtle
headline:

a. rest: $text-color

b. hover: $text-color-subtle
author name, when link (covered in linked ticket so no need to override but confirm as part of this ticket):

a. rest: $text-color

b. hover: $text-color-subtle

Test Steps

Checkout this branch git checkout THEMES-1195
Run fusion repo with linked blocks npx fusion start -f -l @wpmedia/extra-large-manual-promo-block,@wpmedia/extra-large-promo-block,@wpmedia/large-manual-promo-block,@wpmedia/large-promo-block,@wpmedia/medium-manual-promo-block,@wpmedia/medium-promo-block,@wpmedia/small-manual-promo-block,@wpmedia/small-promo-block
add all the blocks and inspect on hover to check the color

Dependencies or Side Effects

Feature pack pr for local development: https://github.com/WPMedia/arc-themes-feature-pack/pull/431
Components pr. for local development: updated link color and font-family arc-themes-components#199

Author Checklist

The author of the PR should fill out the following sections to ensure this PR is ready for review.

Confirmed all the test steps a reviewer will follow above are working.
Confirmed there are no linter errors. Please run npm run lint to check for errors. Often, npm run lint:fix will fix those errors and warnings.
Ran this code locally and checked that there are not any unintended side effects. For example, that a CSS selector is scoped only to a particular block.
Confirmed this PR has reasonable code coverage. You can run npm run test:coverage to see your progress.
- Confirmed this PR has unit test files
- Ran npm run test, made sure all tests are passing
- If the amount of work to write unit tests for this change are excessive,
  please explain why (so that we can fix it whenever it gets refactored).
Confirmed relevant documentation has been updated/added.

Reviewer Checklist

The reviewer of the PR should copy-paste this template into the review comments on review.

Linting code actions have passed.
Ran the code locally based on the test instructions.
- I don’t think this is needed to be tested locally. For example, a padding style change (storybook?) or a logic change (write a test).
I am a member of the engine theme team so that I can approve and merge this. If you're not on the team, you won't have access to approve and merge this pr.
Looked to see that the new or changed code has code coverage, specifically. We want the global code coverage to keep on going up with targeted testing.

vgalatro

I made comments on the extra large manual promo, but they apply to all the promo blocks. We want to make sure we're using the "components" styling to keep things consistent and make sure inheritance is working correctly.

vgalatro · 2023-07-10T14:46:36Z

blocks/extra-large-manual-promo-block/_index.scss

+	&__overline {
+		&:hover {
+			@include scss.block-properties("xl-manual-promo-overline-hover");
+			@include scss.block-components("xl-manual-promo-overline-hover");
+		}
+	}
+	&__headline {
+		&:hover {
+			@include scss.block-properties("xl-manual-promo-headline-hover");
+			@include scss.block-components("xl-manual-promo-headline-hover");
+		}
+	}


We shouldn't need new tokens for the hover state specifically, just for overline and headline.

@vgalatro Fixed this removing hover from here

vgalatro · 2023-07-10T14:57:38Z

blocks/extra-large-manual-promo-block/_index.scss

+			@include scss.block-components("xl-manual-promo-headline-hover");
+		}
+	}
+	&__imgWithLink {


This class should target the image, whether or not it is linked. We can use the "components" styling to then target the link in the image block.

@vgalatro Fixed this

vgalatro · 2023-07-10T14:58:31Z

blocks/extra-large-manual-promo-block/themes/news.json

+			"desktop": {}
+		}
+	},
+	"xl-manual-promo-img-with-link": {


As noted above, this should be changed to always target the image.

@vgalatro Fixed this by adding image component to this

vgalatro · 2023-07-10T14:59:13Z

blocks/extra-large-manual-promo-block/features/extra-large-manual-promo/default.jsx

@@ -131,7 +135,10 @@ const ExtraLargeManualPromo = ({ customFields }) => {
 									onClick={registerSuccessEvent}
 									assistiveHidden={showHeadline && showDescription}
 								>
-									<Image {...imageParams} />
+									<Image
+										className={`${linkURL ? `${BLOCK_CLASS_NAME}__imgWithLink` : ""}`}


Remove this conditional and always add a classname here, ${BLOCK_CLASS_NAME}__img

@vgalatro updated this

vgalatro · 2023-07-10T15:00:32Z

blocks/extra-large-manual-promo-block/themes/news.json

+	"xl-manual-promo-img-with-link": {
+		"styles": {
+			"default": {
+				"opacity": 0.8


This should work as

"components": { "link-hover": { "opacity": 0.8 } }

@vgalatro Instead of link-hover, it's working with image

vgalatro

This is looking better, but I still found some issues. I made notes on the extra large promo block, but they apply to all blocks. We should be able to eliminate the *-promo-overline tokens, cutting the complexity of the CSS we generate.

vgalatro · 2023-07-20T16:57:35Z

blocks/extra-large-manual-promo-block/_index.scss

+		@include scss.block-properties("xl-manual-promo-overline-hover");
+		@include scss.block-components("xl-manual-promo-overline-hover");
+	}
+	&__headline {
+		@include scss.block-properties("xl-manual-promo-headline-hover");
+		@include scss.block-components("xl-manual-promo-headline-hover");


Remove -hover from these token names.

vgalatro · 2023-07-20T17:14:02Z

blocks/extra-large-manual-promo-block/themes/news.json

+		"styles": {
+			"default": {
+				"components": {
+					"heading": {


This should be link-hover, not heading. This is making regular text headings grey.

vgalatro · 2023-07-20T17:46:02Z

blocks/extra-large-manual-promo-block/themes/news.json

+				"link-hover": {
+					"opacity": 0.8
+				}


Needs to be wrapped in the "components" : { } property.

In the news.json files for the other blocks the "image" property also needs to be changed to "link-hover"

vgalatro · 2023-07-20T17:50:45Z

blocks/extra-large-manual-promo-block/themes/news.json

+			"desktop": {}
+		}
+	},
+	"xl-manual-promo-headline-hover": {


Make sure to remove -hover from the token name here too.

Since we need to style the link that's a child of the heading component, we'll still need the *-promo-headline tokens for the blocks.

vgalatro · 2023-07-20T17:59:47Z

blocks/extra-large-manual-promo-block/themes/news.json

+	"xl-manual-promo-overline-hover": {
+		"styles": {
+			"default": {
+				"components": {
+					"overline-hover": {
+						"color": "var(--text-color-subtle)"
+					}
+				}
+			},
+			"desktop": {}
+		}
+	},


The more I look at it, I don't think we need this token at all. Since all we need to do is style the component itself the "overline-hover" rule can be a part of "xl-manual-promo".

vgalatro · 2023-07-20T18:01:02Z

blocks/extra-large-manual-promo-block/themes/news.json

 					"link": {
 						"font-family": "inherit",
 						"font-size": "inherit",
 						"font-weight": "inherit",
 						"line-height": "inherit"
 					},
+					"overline": {
+						"color": "var(--text-color)"
+					},


To go with the other comment, we can move

"overline-hover": { "color": "var(--text-color-subtle)" }

here, and eliminate the *-promo-overline tokens.

vgalatro · 2023-07-20T18:03:18Z

blocks/extra-large-manual-promo-block/features/extra-large-manual-promo/default.jsx

@@ -96,12 +96,16 @@ const ExtraLargeManualPromo = ({ customFields }) => {
 	return availableOverline || availableHeadline || availableImageURL || availableDescription ? (
 		<LazyLoad enabled={shouldLazyLoad}>
 			<article className={BLOCK_CLASS_NAME}>
-				{availableOverline ? <Overline href={overlineURL}>{availableOverline}</Overline> : null}
+				{availableOverline ? (
+					<Overline href={overlineURL} className={`${BLOCK_CLASS_NAME}__overline`}>


No longer need the className here.

vgalatro · 2023-07-20T18:03:32Z

blocks/extra-large-manual-promo-block/_index.scss

+	&__overline {
+		@include scss.block-properties("xl-manual-promo-overline-hover");
+		@include scss.block-components("xl-manual-promo-overline-hover");
+	}


This token may be removed.

malavikakoppula · 2023-07-21T13:52:55Z

This is looking better, but I still found some issues. I made notes on the extra large promo block, but they apply to all blocks. We should be able to eliminate the *-promo-overline tokens, cutting the complexity of the CSS we generate.

@vgalatro, I removed all the hover tokens and adjusted styles according to links.

vgalatro

I think there may have been some confusion when we were talking about the changes needed. We still need a token targeting the images in each promo block, with the opacity property set to the link-hover component there. The way it is now, all of the links change opacity on hover, making them the incorrect color.

vgalatro · 2023-07-25T19:08:21Z

blocks/extra-large-manual-promo-block/themes/news.json

-	"xl-manual-promo-img": {
-		"styles": {
-			"default": {
-				"link-hover": {
-					"opacity": 0.8
-				}
-			},
-			"desktop": {}
-		}


Sorry if my comments caused some confusion, but we still need a token for the image container. The opacity property affects text as well, so the links are too light.

The change needed here was to add the components wrapper like so:

"xl-manual-promo-img": { "styles": { "default": { "components": { "link-hover": { "opacity": 0.8 } } }, "desktop": {} } }

@vgalatro, Fixed it in a622ba2

vgalatro · 2023-07-25T19:08:52Z

blocks/extra-large-manual-promo-block/themes/news.json

@@ -28,49 +28,22 @@
 						"justify-content": "center",
 						"line-height": "var(--heading-level-1-line-height)"
 					},
+					"link-hover": {
+						"color": "var(--text-color-subtle)",
+						"opacity": 0.8


Adding opacity here is making the links too light of a color.

@vgalatro, Fixed it in 8a050cf

vgalatro · 2023-07-25T19:09:06Z

blocks/extra-large-manual-promo-block/features/extra-large-manual-promo/default.jsx

@@ -131,7 +127,6 @@ const ExtraLargeManualPromo = ({ customFields }) => {
 									component={Link}
 									condition={linkURL}
 									href={formatURL(linkURL)}
-									className={`${BLOCK_CLASS_NAME}__img`}


We'll still need this class.

@vgalatro Fixed it in a622ba2

vgalatro · 2023-07-25T19:09:52Z

blocks/extra-large-manual-promo-block/_index.scss

-	&__img {
-		@include scss.block-properties("xl-manual-promo-img");
-		@include scss.block-components("xl-manual-promo-img");
-	}


Keep the image token so we can apply the opacity to the correct element.

@vgalatro Fixed it in a622ba2

vgalatro

Everything is working, however I noticed there seems to be copy/paste errors in the token definitions inside the scss files. Once those are fixed I can approve this PR.

vgalatro · 2023-07-26T14:38:57Z

blocks/extra-large-promo-block/_index.scss

+		@include scss.block-properties("xl-manual-promo-img");
+		@include scss.block-components("xl-manual-promo-img");


These should be xl-promo-img.

@vgalatro Thanks, fixed it 808d50b

vgalatro · 2023-07-26T14:39:37Z

blocks/large-manual-promo-block/_index.scss

+		@include scss.block-properties("xl-manual-promo-img");
+		@include scss.block-components("xl-manual-promo-img");


These should be large-manual-promo-img.

@vgalatro fixed it 808d50b

vgalatro · 2023-07-26T14:40:03Z

blocks/large-promo-block/_index.scss

+		@include scss.block-properties("xl-manual-promo-img");
+		@include scss.block-components("xl-manual-promo-img");


These should be large-promo-img.

@vgalatro fixed it 808d50b

vgalatro · 2023-07-26T14:40:28Z

blocks/medium-manual-promo-block/_index.scss

+		@include scss.block-properties("xl-manual-promo-img");
+		@include scss.block-components("xl-manual-promo-img");


These should be medium-manual-promo-img

@vgalatro fixed it 808d50b

vgalatro · 2023-07-26T14:40:50Z

blocks/medium-promo-block/_index.scss

+		@include scss.block-properties("xl-manual-promo-img");
+		@include scss.block-components("xl-manual-promo-img");


These should be medium-promo-img

@vgalatro fixed it 808d50b

vgalatro · 2023-07-26T14:41:09Z

blocks/small-manual-promo-block/_index.scss

+		@include scss.block-properties("xl-manual-promo-img");
+		@include scss.block-components("xl-manual-promo-img");


These should be small-manual-promo-img

vgalatro · 2023-07-26T14:41:27Z

blocks/small-promo-block/_index.scss

+		@include scss.block-properties("xl-manual-promo-img");
+		@include scss.block-components("xl-manual-promo-img");


These should be small-promo-img

malavikakoppula · 2023-07-26T15:40:15Z

Everything is working, however I noticed there seems to be copy/paste errors in the token definitions inside the scss files. Once those are fixed I can approve this PR.

@vgalatro Fixed all of these 808d50b

vgalatro

All the changes look good, approved!

pardistaherzadeh · 2023-07-26T22:56:38Z

Looks good to me. Ready to be merged. @vgalatro

Updated headline, overline and image opacity

e970a24

malavikakoppula changed the title ~~Updated headline, overline and image opacity~~ THEMES-1195 Updated headline, overline and image opacity Jun 29, 2023

malavikakoppula added the WIP Work in progress. Feel free to give feedback, but no formal review is requested at this time. label Jun 29, 2023

Malavika Koppula added 3 commits July 5, 2023 10:09

Fixed image opacity

ae68bc3

removed console statements

d97bec3

Merge branch 'arc-themes-release-version-2.0.3' into THEMES-1195

068ec0d

malavikakoppula added ready for review The PR author has completed the PR template and is ready for a review and removed WIP Work in progress. Feel free to give feedback, but no formal review is requested at this time. labels Jul 5, 2023

malavikakoppula marked this pull request as ready for review July 5, 2023 14:29

malavikakoppula requested a review from a team as a code owner July 5, 2023 14:29

Malavika Koppula and others added 2 commits July 5, 2023 10:40

Merge branch 'arc-themes-release-version-2.0.3' into THEMES-1195

3c4f616

Merge branch 'arc-themes-release-version-2.0.3' into THEMES-1195

45a8c15

vgalatro added review in progress A review is underway. Even if an approval has been submitted, wait for all reviews to be completed. and removed ready for review The PR author has completed the PR template and is ready for a review labels Jul 10, 2023

vgalatro self-assigned this Jul 10, 2023

vgalatro suggested changes Jul 10, 2023

View reviewed changes

vgalatro added needs changes The reviewer has requested changes from the PR author and removed review in progress A review is underway. Even if an approval has been submitted, wait for all reviews to be completed. labels Jul 10, 2023

Malavika Koppula added 5 commits July 11, 2023 22:59

Merge branch 'arc-themes-release-version-2.0.3' into THEMES-1195

59d248f

Merge branch 'arc-themes-release-version-2.0.3' into THEMES-1195

741a1fa

Fixed hover state and images with link

21ef594

Merge branch 'arc-themes-release-version-2.0.3' into THEMES-1195

5b9ad1d

chnaged imgWithLink to img

8f770d0

malavikakoppula added additional review The PR author has requested multiple reviewers. Do not merge until at least 2 approvals are complete and removed needs changes The reviewer has requested changes from the PR author labels Jul 19, 2023

vgalatro added review in progress A review is underway. Even if an approval has been submitted, wait for all reviews to be completed. and removed additional review The PR author has requested multiple reviewers. Do not merge until at least 2 approvals are complete labels Jul 20, 2023

vgalatro suggested changes Jul 20, 2023

View reviewed changes

vgalatro added needs changes The reviewer has requested changes from the PR author and removed review in progress A review is underway. Even if an approval has been submitted, wait for all reviews to be completed. labels Jul 20, 2023

Merge branch 'arc-themes-release-version-2.0.3' into THEMES-1195

0485297

malavikakoppula added additional review The PR author has requested multiple reviewers. Do not merge until at least 2 approvals are complete and removed needs changes The reviewer has requested changes from the PR author labels Jul 21, 2023

vgalatro suggested changes Jul 25, 2023

View reviewed changes

vgalatro added needs changes The reviewer has requested changes from the PR author and removed review in progress A review is underway. Even if an approval has been submitted, wait for all reviews to be completed. labels Jul 25, 2023

Malavika Koppula added 3 commits July 26, 2023 09:45

reverted img token and class

a622ba2

Merge branch 'arc-themes-release-version-2.0.3' into THEMES-1195

cad4b5a

Removed opacity from link-hover

8a050cf

malavikakoppula added additional review The PR author has requested multiple reviewers. Do not merge until at least 2 approvals are complete and removed needs changes The reviewer has requested changes from the PR author labels Jul 26, 2023

removed opacity from link-hover

9036af9

vgalatro added review in progress A review is underway. Even if an approval has been submitted, wait for all reviews to be completed. and removed additional review The PR author has requested multiple reviewers. Do not merge until at least 2 approvals are complete labels Jul 26, 2023

vgalatro suggested changes Jul 26, 2023

View reviewed changes

Updated token definitions

808d50b

vgalatro approved these changes Jul 26, 2023

View reviewed changes

vgalatro removed their assignment Jul 26, 2023

vgalatro added ready for design qa Development complete and approved, ready for design to review and removed review in progress A review is underway. Even if an approval has been submitted, wait for all reviews to be completed. labels Jul 26, 2023

Merge branch 'arc-themes-release-version-2.0.3' into THEMES-1195

884b849

malavikakoppula merged commit c8e2634 into arc-themes-release-version-2.0.3 Jul 27, 2023
6 checks passed

malavikakoppula deleted the THEMES-1195 branch July 27, 2023 15:36

		@include scss.block-properties("xl-manual-promo-img");
		@include scss.block-components("xl-manual-promo-img");

THEMES-1195 Updated headline, overline and image opacity #1690

THEMES-1195 Updated headline, overline and image opacity #1690

Conversation

malavikakoppula commented Jun 29, 2023 • edited Loading

Description

Jira Ticket

Acceptance Criteria

Test Steps

Dependencies or Side Effects

Author Checklist

Reviewer Checklist

vgalatro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgalatro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malavikakoppula commented Jul 21, 2023

vgalatro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgalatro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malavikakoppula commented Jul 26, 2023

vgalatro left a comment

Choose a reason for hiding this comment

pardistaherzadeh commented Jul 26, 2023

malavikakoppula commented Jun 29, 2023 •

edited

Loading