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

[EuiProvider] Configurable enforcement of provider usage #6216

Merged
merged 14 commits into from
Sep 14, 2022

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Sep 7, 2022

Summary

#6210

Consumers can set a module variable to 'log', 'warn', or 'error' with the setEuiDevProviderWarning method to capture instances where components are accessing the EUI theme (via useEuiTheme) but not using EuiProvider.

Checklist

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6216/

@thompsongl
Copy link
Contributor Author

Pinging @chandlerprall for an early look

@thompsongl thompsongl marked this pull request as ready for review September 9, 2022 15:04
@thompsongl
Copy link
Contributor Author

Another thought is to avoid the shape change and introduce Yet Another context provider

After more thought, not sure I like this idea so much

Although I'm not too concerned because no APIs would change (the global var would work the same way).

Comfortable moving forward with the current pattern and adjusting later as needed.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6216/

src/services/theme/context.ts Outdated Show resolved Hide resolved
src-docs/src/index.js Outdated Show resolved Hide resolved
@thompsongl
Copy link
Contributor Author

thompsongl commented Sep 9, 2022

The same check needs to be made for WithEuiTheme

withEuiTheme uses useEuiTheme so it works

What do you think of exporting computedTheme from this file and doing a strict equality check against the theme from the context? That would avoid the shape change and not need a new provider

Great call! I'll try this.

@chandlerprall
Copy link
Contributor

withEuiTheme uses useEuiTheme so it works

Yep, sure does! Don't know how I missed that when I checked.

src/services/theme/context.ts Outdated Show resolved Hide resolved
src/services/theme/provider.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6216/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; tested with the example toast in various configurations

@thompsongl
Copy link
Contributor Author

@clintandrewhall Does this still look ok after the move from a global var to a module var?

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6216/

@thompsongl
Copy link
Contributor Author

CI failures will be resolved when I revert the test docs change

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

LGTM. We'll like follow this pattern with any Kibana context requirements.

Thanks for jumping on this so quickly! Shared UX will coordinate once this is released and in Kibana, just let us know.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6216/

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.

4 participants