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

fix(flags): support multiple children prop in PostHogFeature #1516

Merged
merged 5 commits into from
Nov 13, 2024

Conversation

havenbarnes
Copy link
Contributor

@havenbarnes havenbarnes commented Nov 8, 2024

Changes

Closes PostHog/posthog#22922

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

Copy link

vercel bot commented Nov 8, 2024

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

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Nov 8, 2024 8:33pm

@havenbarnes
Copy link
Contributor Author

havenbarnes commented Nov 8, 2024

@neilkakkar hello! I see you looked at this ticket a bit when it first came in - would love your thoughts on this when you get back.

Would we prefer a non-backwards compat change like this, or perhaps split brained logic that removes the wrapper div only if multiple children are passed in?

My opinion was to prefer the former just to keep the behavior simpler for SDK users, but I could see some arguments for the latter if we know of any use case for the wrapper div that make this change problematic

Edit: I thought of a way to keep this generally backwards compatible

Copy link

github-actions bot commented Nov 8, 2024

Size Change: 0 B

Total Size: 3.01 MB

ℹ️ View Unchanged
Filename Size
dist/all-external-dependencies.js 193 kB
dist/array.full.es5.js 251 kB
dist/array.full.js 342 kB
dist/array.full.no-external.js 341 kB
dist/array.js 168 kB
dist/array.no-external.js 167 kB
dist/dead-clicks-autocapture.js 12.8 kB
dist/exception-autocapture.js 8.8 kB
dist/external-scripts-loader.js 2.19 kB
dist/main.js 169 kB
dist/module.full.js 342 kB
dist/module.full.no-external.js 341 kB
dist/module.js 168 kB
dist/module.no-external.js 167 kB
dist/recorder-v2.js 102 kB
dist/recorder.js 103 kB
dist/surveys-preview.js 56.7 kB
dist/surveys.js 62.1 kB
dist/tracing-headers.js 1.33 kB
dist/web-vitals.js 10.3 kB

compressed-size-action

Copy link
Contributor

@dmarticus dmarticus left a comment

Choose a reason for hiding this comment

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

this seems cool to me; nice work! Don't forget to the add the bump: $version label to this PR to automatically bump and publish the latest version of these changes once they've been merged.

@havenbarnes havenbarnes added the bump patch Bump patch version when this PR gets merged label Nov 12, 2024
@havenbarnes havenbarnes merged commit df21dc2 into main Nov 13, 2024
16 checks passed
@havenbarnes havenbarnes deleted the multi-child-posthog-feature branch November 13, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React - Content is wrapped within a div when the feature flag is enabled
2 participants