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.

Sign up for GitHub

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

Jump to bottom

Add theme-aware Global component #2385

Merged
merged 10 commits into from
Jan 22, 2023
Merged

Add theme-aware Global component #2385

merged 10 commits into from
Jan 22, 2023

Conversation

lachlanjc
Copy link
Member

@lachlanjc lachlanjc commented Jan 16, 2023 •
edited by hasparus
Loading

At long last, we no longer have to recommend using Emotion's Global component if you need page-specific global CSS. The API:

 <Global sx={{ h1: { color: 'primary', fontSize: 3 } }} />

@hasparus: While it's working fine in projects, in the test environment, I'm getting scoped class names from Emotion, which is causing the tests to fail. I've commented those out, but if you look at the snapshot you can see what's happening—any ideas appreciated so it has full test coverage. Both Emotion's tests & MUI's tests somehow don't run into this issue; I can't tell what's different in our environment.

Version

Published prerelease version: v0.15.5-develop.1

Changelog

🎉 This release contains work from a new contributor! 🎉

Thank you, Nischal Shakya (@Nischal2015), for all your work!

🐛 Bug Fix

🏠 Internal

Authors: 3

@lachlanjc lachlanjc added enhancement New feature or request minor Increment the minor version when merged labels Jan 16, 2023
@lachlanjc lachlanjc linked an issue Jan 16, 2023 that may be closed by this pull request
Provide Theme UI-specific "Global" component for global styles #2207
Closed
@vercel
Copy link

vercel bot commented Jan 16, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
theme-ui ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 22, 2023 at 6:06PM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 16, 2023 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ac36c53:

Sandbox Source
next-theme-ui-example Configuration
gatsby-plugin-theme-ui-example Configuration

@hasparus
Copy link
Member

I'll take a look at those tests.

hasparus
hasparus reviewed Jan 22, 2023
View reviewed changes
packages/global/src/index.tsx Outdated
jsx(EmotionGlobal, {
styles: (emotionTheme: unknown) => {
const theme = emotionTheme as Theme
return css(sx)(theme)
Copy link
Member

Choose a reason for hiding this comment

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

I'm kinda wondering if

useMemo(() => css(sx)(theme), [sx, theme])

here would be an overkill.
We can always add it later after we know more I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good call. I wish evaluating perf with hooks like that with were easier…conceptually makes sense to use it at least

hasparus
hasparus reviewed Jan 22, 2023
View reviewed changes
examples/next/lib/theme.ts Outdated
@@ -1,6 +1,6 @@
import { makeTheme } from '@theme-ui/css/utils'
// import { makeTheme } from '@theme-ui/css/utils'
Copy link
Member

Choose a reason for hiding this comment

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

After running pnpm build Next.js dev server doesn't catch that the libraries are built instantly, so I removed .next dir in the example, restarted pnpm dev and the error on this import went away.

The error was basically saying we can't import TypeScript files directly, and that's totally fine, because we want to develop examples against prebuilt libraries, but even after rebuilding you gotta clean the cache if you started dev server previously.

@hasparus
Copy link
Member

Don't we want to mark this as patch if it's non-breaking and we're still in 0.x version?

image

@hasparus
Copy link
Member

hasparus commented Jan 22, 2023 •
edited
Loading

I've sent a PR against your branch with a change motivated by the failing test here. — #2386.

@lachlanjc lachlanjc added patch Increment the patch version when merged and removed minor Increment the minor version when merged labels Jan 22, 2023
@hasparus
Copy link
Member

PR release is failing but it's not this branch that broke it, so I'd merge.

image

@hasparus hasparus merged commit f9c7f93 into develop Jan 22, 2023
@hasparus hasparus deleted the 2207-global-component branch January 22, 2023 18:27
@hasparus hasparus added the prerelease This change is available in a prerelease. label Jan 22, 2023
@hasparus hasparus mentioned this pull request Feb 7, 2023
Merged
@hasparus
Copy link
Member

hasparus commented Feb 7, 2023

🚀 PR was released in v0.15.5 🚀

@hasparus hasparus added released This issue/pull request has been released. and removed prerelease This change is available in a prerelease. labels Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hasparus hasparus hasparus approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request patch Increment the patch version when merged released This issue/pull request has been released.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide Theme UI-specific "Global" component for global styles
2 participants
@lachlanjc @hasparus