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

SCSS mixins: long-content-fade to apply gradient from transparent to color #65598

Merged
merged 2 commits into from
Aug 8, 2022

Conversation

zaguiini
Copy link
Contributor

@zaguiini zaguiini commented Jul 14, 2022

Proposed Changes

Sync our version of long-content-fade with Gutenberg's.

This PR will be good to go once the next version of @wordpress/base-styles is merged. This PR will also bump the package's version.

Testing Instructions

Open Calypso, click Switch Sites and you should see the gradient working on the edge for long site URLs. Amazingly, this does not work on trunk:

This PR trunk
image image

Related to #65505 (comment).

@github-actions
Copy link

github-actions bot commented Jul 14, 2022

@zaguiini zaguiini changed the title Update overflow-gradient to use transparent Shared SCSS: overflow-gradient to apply gradient from transparent to color Jul 14, 2022
@zaguiini zaguiini changed the title Shared SCSS: overflow-gradient to apply gradient from transparent to color Shared SCSS: long-content-fade to apply gradient from transparent to color Jul 14, 2022
@zaguiini zaguiini changed the title Shared SCSS: long-content-fade to apply gradient from transparent to color SCSS mixins: long-content-fade to apply gradient from transparent to color Jul 14, 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.

@zaguiini zaguiini requested a review from a team July 14, 2022 14:27
@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 14, 2022
@zaguiini zaguiini added [Status] Blocked / Hold DO NOT MERGE and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 14, 2022
@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 14, 2022
@zaguiini zaguiini self-assigned this Jul 14, 2022
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 you described, I had tested the gutenberg one before also. The screenshots almost had me fooled 😅 . I was looking at the search bar wondering whats the difference, it draws the attention because its highlighted.

@wojtekn
Copy link
Contributor

wojtekn commented Jul 25, 2022

Looks good and works as expected. Just a note - currently the other branch has a conflict. I needed to solve conflicts after merging update/search-fade-use-css-variable-as-color to this one.

@wojtekn wojtekn self-requested a review July 25, 2022 08:56
@zaguiini zaguiini force-pushed the update/long-content-fade-from-transparent-to-color branch from 9401533 to 978ecdb Compare August 4, 2022 18:29
@zaguiini zaguiini marked this pull request as ready for review August 4, 2022 18:29
@zaguiini zaguiini requested review from a team, vykes-mac, wojtekn and noahtallen August 4, 2022 18:29
@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/long-content-fade-from-transparent-to-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.

This tests well for me. 👍

Why can't we use the Gutenberg version directly, though?

@zaguiini
Copy link
Contributor Author

zaguiini commented Aug 8, 2022

For a couple of reasons, @noahtallen:

  1. Gutenberg's version is inside a file called _mixins.scss that exposes a bunch of other mixins, some that would probably clash with the ones used in Calypso;
  2. Despite the implementation being exactly the same, the default value for the color argument differs. Gutenberg's is #fff while Calypso's is var( --color-surface-rgb ). This might be potentially problematic as we would need to go through all cases where this mixin is used to understand better the implications and compare the output.

It can be done, but I'd prefer to do that on a separate PR. We'd first need to isolate the long-content-fade mixin from Gutenberg and that'll take a while since the package was released a few days ago. Created an issue: #66350.

@zaguiini zaguiini merged commit 0081be8 into trunk Aug 8, 2022
@zaguiini zaguiini deleted the update/long-content-fade-from-transparent-to-color branch August 8, 2022 15:46
@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

Cool! That makes sense to me.

One thing that comes to mind is that the way Sass imports work will be changing in the near future. I believe that Gutenberg is not yet compatible with the new approach, nor is Calypso. But in the new system, imports are now namespaced, so using just one mixin by itself should be possible without impacting any other mixins.

Upgrading Gutenberg and Calypso to support that has been on my todo list for a while, but there have been other priorities :)

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.

5 participants