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

feat: Add withConfigProvider to facilitate removal of wrapping every single component #286

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ajenkins-mparticle
Copy link
Contributor

@ajenkins-mparticle ajenkins-mparticle commented Jun 13, 2024

Instructions

  1. PR target branch should be against main
  2. PR title name should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-title-check.yml
  3. PR branch prefix should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-branch-check-name.yml

Summary

Nearly all components currently in the aquarium (not all, just most) follow the same pattern of wrapping antd without modification, and exporting to consumers. The one functionality that is provided, however, is wrapping every individual antd component in Antd's ConfigProvider with mParticle's theme.

We would not have to wrap antd components and re-export them wrapped in an mParticle ConfigProvider if the consumer had a ConfigProvider at it's root where the consumed component was being rendered.

This initial PR will have several follow up PRs in consuming applications and also in aquarium. Namely to:

  1. Create a global storybook decorator that wraps all our stories
  2. Use the withConfigProvider in various consuming UIs to provide a root mParticle ConfigProvider whereever an aquairum component is used
  3. Delete all "wrapped but not extended" components and stories and replace them with direct antd exports (this may require updating type annotations in consuming applications to facilitate them upgrading. Basically, we'd no longer have to do things like this: feat: wrap radio group and button components #282, we'd get all of antd for free in consuming applications.

Technically, withConfigProvider isn't even needed at all, consuming applications can just use <ConfigProvider> at the root of their react tree. It's (arguably) a nice helper utility though to facilitate the removal of the responsibility of every single component to be wrapped and helps start the conversation

Testing Plan

  • Was this tested locally? If not, explain why.
  • {explain how this has been tested, and what, if any, additional testing should be done}

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

@@ -8,6 +8,15 @@ export const ConfigProvider = (props: IConfigProviderProps) => {
return <AntConfigProvider {...props} theme={LightTheme} />
}

export const withConfigProvider =
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you want this avaliable in userland, itll have to be exported from src/components/index.ts as well

@jared-dickman
Copy link
Collaborator

consuming applications can just use at the root of their react tree

this is tricky for us since only 1 of our 3 products has a real root react tree [cortex]
nancy has a root tree of aurelia
and indicative has a root tree of angularjs

:thisisfine: :hidethepain:

@ajenkins-mparticle
Copy link
Contributor Author

ajenkins-mparticle commented Jun 13, 2024

consuming applications can just use at the root of their react tree

this is tricky for us since only 1 of our 3 products has a real root react tree [cortex] nancy has a root tree of aurelia and indicative has a root tree of angularjs

:thisisfine: :hidethepain:

Yeah, agree.

@tibuurcio made a suggestion that I like for Nancy which is, in our aurelia react wrapper we wrap whatever component is passed into it with withConfigProvider which means we should be able to isolate the change in nancy, in theory, to a single place.

I haven't investigated the other 2 indicative, yet.

EDIT:

Made a draft, this should work: https://git.corp.mparticle.com/mParticle/mPServer/compare/config-provider?expand=1

EDIT 2:

Made an indicative draft: https://git.corp.mparticle.com/mParticle/indicative-web/compare/indicative-with-aquarium-config-provider?expand=1

Indicative would have to update everywhere where ReactDOM.render is called, which is a lot of places, but the core function that does the work is a tiny one 😅

EDIT 3:

Cortex was easy: https://git.corp.mparticle.com/mParticle/cortex-rails-api/compare/wrap-with-config-provider?expand=1

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.

None yet

2 participants