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

Tip: Use CSS-in-JS #26366

Closed
wants to merge 5 commits into from
Closed

Conversation

sarayourfriend
Copy link
Contributor

Description

This PR updates the Tip component to use CSS-in-JS similar to how newer components do like the AnglePickerControl and Text.

How has this been tested?

I've confirmed there are no visual or behavioral differences between this branch and upstream in storybook.

Screenshots

Before:
Captura de Tela 2020-10-07 às 13 10 21

After:
Captura de Tela 2020-10-07 às 13 10 18

Types of changes

Non-breaking style change. The classname is preserved so it is still possible to target the components-tip class.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@sarayourfriend sarayourfriend self-assigned this Oct 21, 2020
@sarayourfriend sarayourfriend mentioned this pull request Oct 21, 2020
6 tasks
@sarayourfriend sarayourfriend force-pushed the refactor/use-css-in-js-for-tip branch from 9b9ddca to 951a081 Compare October 21, 2020 17:56
@sarayourfriend sarayourfriend changed the title Refactor/use css in js for tip Tip: Use CSS-in-JS Oct 21, 2020
/**
* External dependencies
*/
import { jsx } from '@emotion/core';
Copy link

Choose a reason for hiding this comment

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

@saramarcondes Haiii!!!

Interesting idea! From my experience (in figuring out things in G2...), I personally would avoid the jsx pragma based approach. Theme UI offers a similar API.

In practice, I found it to be pretty clunky (and slightly jarring if you're unfamiliar with the workflow).

I believe I understand why this was attempted ❤️ .
My guess was to avoid using styled :) - which has a great dev workflow, with a slight trade-off in performance.

I would recommend continuing to use styled as your continuing your CSS-in-JS journey within Gutenberg.
It would be consistent with the current (but limited) implementation.

I think introducing the JSX pragma may cause some fragmentation in tidying and refactoring things.

@ItsJonQ
Copy link

ItsJonQ commented Oct 21, 2020

It looks like the E2E tests are unhappy with TypeScript things 😱
I believe I saw another PR with updates.. Something with tinycolor2 types? :)

@ItsJonQ ItsJonQ mentioned this pull request Oct 21, 2020
6 tasks
@sarayourfriend sarayourfriend mentioned this pull request Oct 22, 2020
6 tasks
@sarayourfriend
Copy link
Contributor Author

Fixing the tests are blocked by #26078

@sarayourfriend sarayourfriend force-pushed the refactor/use-css-in-js-for-tip branch from b67e346 to 1c9d31b Compare October 27, 2020 15:13
@ItsJonQ
Copy link

ItsJonQ commented Oct 28, 2020

@saramarcondes Thank you following up and adjusting the implementation (styled vs css). Hopefully the concerns I raised made sense 🙏 .

With that, the it's a 👍 from me.

However, it looks like the tests are unhappy. Hopefully #26078 can help resolve it!

I left my comments in that PR. My TypeScript knowledge isn't good enough to thoughtfully and accurately go over those changes 😅 .

@gziolo gziolo added the [Package] Components /packages/components label Nov 8, 2020
@sarayourfriend
Copy link
Contributor Author

Closing in favor of focusing on G2-components integration.

@aristath aristath deleted the refactor/use-css-in-js-for-tip branch November 10, 2020 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants