Skip to content
This repository has been archived by the owner on Jan 24, 2025. It is now read-only.

fix(docz-theme-default): rename playground container to avoid conflicts #429

Merged
merged 1 commit into from
Oct 30, 2018

Conversation

beerose
Copy link
Contributor

@beerose beerose commented Oct 28, 2018

Description

Solves issue: #426
I renamed playground container to avoid conflicts with components named App.

Copy link
Contributor

@Jared-Dev Jared-Dev left a comment

Choose a reason for hiding this comment

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

Nice changes! Thoughts on further namespacing that variable? While it is unlikely I could potentially see someone naming something with a single leading underscore.

@@ -388,13 +388,13 @@ export class Render extends Component<RenderComponentProps, RenderState> {

private transformCode(code: string): string {
return `
const App = ({ children }) => (
const _App = ({ children }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const _App = ({ children }) => (
const _Docz_App = ({ children }) => (

What does everyone think about namespacing this so that it is even less likely to collide in the future? Since the user shouldn't need to interact with this directly I can't think of too many negatives to doing this.

<React.Fragment>
{children && typeof children === 'function' ? children() : children}
</React.Fragment>
)

render(<App>${code}</App>)
render(<_App>${code}</_App>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
render(<_App>${code}</_App>)
render(<_Docz_App>${code}</_Docz_App>)

Copy link
Member

Choose a reason for hiding this comment

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

I think that use underscore is something weird, I prefer DoczApp like normal!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pedronauck I renamed it do DoczApp

@pedronauck pedronauck merged commit 9766144 into doczjs:master Oct 30, 2018
@Jared-Dev
Copy link
Contributor

Closes #426

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants