Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Search: match edge fade color with --color-surface variable #65560

Merged
merged 5 commits into from
Aug 8, 2022

Conversation

zaguiini
Copy link
Contributor

@zaguiini zaguiini commented Jul 13, 2022

Proposed Changes

Setting the --color-surface variable in the <Search /> component styling doesn't change the color used in the edge fading gradient at the end of the input:

image

This PR fixes that by matching the color of the --color-surface variable in the long-content-fade mixin call:

image

Testing Instructions

Install dependencies and run yarn workspace @automattic/search run storybook. Check the "With Different Color" story and type a few characters so it overflows the container. The content should fade to the red background instead of white.

Pre-merge Checklist

Complete applicable items on this checklist before merging into trunk. Inapplicable items can be left unchecked.

Both the PR author and reviewer are responsible for ensuring the checklist is completed.

Related to #65505 (comment).

@github-actions
Copy link

github-actions bot commented Jul 13, 2022

@zaguiini zaguiini self-assigned this Jul 13, 2022
@zaguiini zaguiini requested a review from a team July 13, 2022 17:56
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 13, 2022
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Contributor

@vykes-mac vykes-mac left a comment

Choose a reason for hiding this comment

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

Works as expected, Tested in the gutenberg fix also.

Copy link
Contributor

@wojtekn wojtekn left a comment

Choose a reason for hiding this comment

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

Looks good, works as expected. Note for testing steps - the Gutenberg branch is merged now, so we should use trunk when we prepare base-styles for Calypso for testing purposes.

@zaguiini zaguiini force-pushed the update/search-fade-use-css-variable-as-color branch from 3e3b08c to fb8317b Compare August 4, 2022 18:35
@matticbot
Copy link
Contributor

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit update/search-fade-use-css-variable-as-color on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

Copy link
Contributor

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

Pixel peeping a bit, I see small indents from the right and left elements along the top and bottom edges. I think this was already the case before, though. If it's an easy fix, might be worth adding!

image

I assume we have to also use #65598 to have the fade-out working correctly!

packages/search/src/style.scss Outdated Show resolved Hide resolved
@zaguiini
Copy link
Contributor Author

zaguiini commented Aug 8, 2022

Great catch, @noahtallen! That's fixed, please re-review the PR!

@zaguiini zaguiini merged commit f6444d0 into trunk Aug 8, 2022
@zaguiini zaguiini deleted the update/search-fade-use-css-variable-as-color branch August 8, 2022 18:43
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 8, 2022
@noahtallen
Copy link
Contributor

It's looking a lot better! I did notice one bug we should probably fix:

Screen.Recording.2022-08-08.at.1.21.21.PM.mov

If you type in long text, the final characters (and the typing cursor) get hidden and you can't see what you're typing anymore.

@zaguiini
Copy link
Contributor Author

zaguiini commented Aug 8, 2022

I see @noahtallen. It doesn't look like a bug, but definitely needs some love. Could you please create a separate issue for that?

@noahtallen
Copy link
Contributor

#66364

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

Successfully merging this pull request may close these issues.

5 participants