Skip to content

[EuiProvider] Configurable enforcement of provider usage#6216

Merged
chandlerprall merged 14 commits intoelastic:mainfrom
thompsongl:6210-provider
Sep 14, 2022
Merged

[EuiProvider] Configurable enforcement of provider usage#6216
chandlerprall merged 14 commits intoelastic:mainfrom
thompsongl:6210-provider

Conversation

@thompsongl
Copy link
Copy Markdown
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
Copy Markdown

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

@thompsongl
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

@thompsongl
Copy link
Copy Markdown
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
Copy Markdown
Contributor

withEuiTheme uses useEuiTheme so it works

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

@kibanamachine
Copy link
Copy Markdown

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

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@kibanamachine
Copy link
Copy Markdown

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

@thompsongl
Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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
Copy Markdown

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