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

wrap all website examples in ThemeProvider #1353

Merged
merged 2 commits into from
Jun 14, 2023
Merged

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented Jun 13, 2023

Changes

What I did: Changed examples/index.tsx so that all exports are wrapped in <ThemeProvider> using a HOC withThemeProvider. Also some examples were using (nonexistent) args, so removed those too.

Why: In #1351, toasts were not working on website because context was not being found somehow. I figured out it's because context doesn't work in Astro between islands. (Previously, ThemeProvider and the examples were rendered in separate islands). Within the same island, context works fine.

Testing

Tested against #1351 and toasts are now correctly showing up!

Screen.Recording.2023-06-13.at.3.45.04.PM.mov

Docs

N/A

@mayank99 mayank99 self-assigned this Jun 13, 2023
@mayank99 mayank99 requested review from a team as code owners June 13, 2023 19:39
@mayank99 mayank99 requested review from gretanausedaite and LostABike and removed request for a team June 13, 2023 19:39
Copy link
Contributor

@LostABike LostABike left a comment

Choose a reason for hiding this comment

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

I think this search bar is supposed to be longer? (the rest of the bars in the SearchBox doc are also short)
image

Or did we update all input boxes to have a smaller size at some point? 🤔

@mayank99
Copy link
Contributor Author

@LostABike I've fixed the styles issue. It was because there was an additional wrapper element. It's a bit hacky but works. We'll need to find a more robust solution in the future.

The problem was that #1300 introduced an extra <div> in ThemeProvider. If the parent is a flexbox, then this div ends up occupying space. This is making me think it could also cause problems in user apps.

Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

Something weird with backdrop and side nav:
image


export { default as VisuallyHiddenIconExample } from './VisuallyHidden.icon';
export { default as VisuallyHiddenMoreTextExample } from './VisuallyHidden.moretext';
import * as React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a lot of lines 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, but it's a lot fewer lines than adding <ThemeProvider></ThemeProvider> manually to every single example usage - especially when you consider that we will start using examples in other places (like #1352)

@mayank99
Copy link
Contributor Author

Something weird with backdrop and side nav

Good catch, but this means context is actually working now (previously it portaled into <body>). So it's a simple z-index issue. I'll think about how to solve it later; shouldn't block this PR.

@mayank99 mayank99 merged commit 7c025da into dev Jun 14, 2023
@mayank99 mayank99 deleted the mayank/themeprovider-examples branch June 14, 2023 14:04
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.

3 participants