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

General styling: remove long-content-fade SCSS mixin and use the one exported by @wordpress/base-styles #66350

Open
1 of 3 tasks
zaguiini opened this issue Aug 8, 2022 · 6 comments

Comments

@zaguiini
Copy link
Contributor

zaguiini commented Aug 8, 2022

Details

We're defining our own version of the long-content-fade mixin and it's pretty much a copy-paste of what we have in @wordpress/base-styles.

It's a good idea to remove our version and use the one shipped by the agnostic package. That way, we'd have a single source of truth and something that the community maintains, essentially one less point of maintenance for us.

Checklist

  • In @wordpress/base-styles, extract the long-content-fade mixin to its own file so we can import it exclusively
  • Remove the version from Calypso and import the long-content-fade file inside client/assets/stylesheets/shared/mixins/_mixins.scss
  • Check that all usages of long-content-fade did not introduce regressions, including the places where we're not defining the $color variable, which has a different default value in the mixin definition

Related

#65598.

@vykes-mac
Copy link
Contributor

vykes-mac commented Dec 13, 2022

created a PR WordPress/gutenberg#46485. Seems I can't add labels as a first time contributer

@vykes-mac vykes-mac self-assigned this Dec 13, 2022
@p-jackson
Copy link
Member

w.r.t WordPress/gutenberg#46485 see p1671421937566529-slack-C0347E545HR

Is it really necessary to change our upstream dependency? What's the harm of importing a bunch of SCSS mixins if they don't get used? If there are naming collisions perhaps it'd be better for us to change our names to avoid confusion with the GB mixins anyway.

@zaguiini
Copy link
Contributor Author

Are we going to work on this? 👀

@vykes-mac
Copy link
Contributor

I looked at this and the change that is required for this simple thing has gotten huge. I don't think the effort is worth the results.

@autumnfjeld
Copy link
Contributor

Should this be closed? Or moved into the backlog?

@vykes-mac
Copy link
Contributor

Yup I intended to close it

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

No branches or pull requests

4 participants