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

Improve Function Name and Multiline Object Formatting #27

Closed
wants to merge 3 commits into from
Closed

Improve Function Name and Multiline Object Formatting #27

wants to merge 3 commits into from

Conversation

kohlmannj
Copy link
Contributor

@kohlmannj kohlmannj commented Aug 14, 2017

Two little improvements in this PR:

Function Name Formatting: prefer displayName if set

I'm trying to use jsx-to-string with some JSX code that involves passing a React component function as a prop to a React component instance, along with options { useFunctionCode: true, functionNameOnly: true }.

Our use case is a little more complicated than that, insofar as B is a React component that is generated by a Higher-Order Component. Long story short, printing the function's displayName property offers greater clarity than printing its name, which for this HOC, is generic.

Before

Note the ambiguous Themed prop name above. That's the name property of an HOC we're using.

<Responsive as={Themed}
  title='Shared Figure Title'
  breakpoints={{
"(max-width: 539px)": {
  "src": "http://placehold.it/320x160",
  "caption": "For some reason, this is just a captioned image on mobile."
},
"(min-width: 540px)": {
  "caption": "On desktop, this is a looping video. Try resizing the window to see what happens on mobile.",
  "children": <FrameAnimation src='https://video1.nytimes.com/paidpost/belvedere/the-view-from-here/tabletop.mp4' />
}
  }} />

After

Turns out that function has a displayName, based on the wrapped component's displayName. Hence, we can print that instead to offer a bit more clarity.

<Responsive as={ThemedFigure}
  title='Shared Figure Title'
  breakpoints={{
"(max-width: 539px)": {
  "src": "http://placehold.it/320x160",
  "caption": "For some reason, this is just a captioned image on mobile."
},
"(min-width: 540px)": {
  "caption": "On desktop, this is a looping video. Try resizing the window to see what happens on mobile.",
  "children": <FrameAnimation src='https://video1.nytimes.com/paidpost/belvedere/the-view-from-here/tabletop.mp4' />
}
  }} />

Object Formatting: add indents to multi-line formatted object strings

Depending on its content, an object might be stringified as a multi-line string. I noticed this and added a bit of code to conditionally add indentation to all but the first line of the multi-string value in this case.

Before

The breakpoints prop is a multi-line object, but it doesn't have the correct indentation below.

<Responsive as={ThemedFigure}
  title='Shared Figure Title'
  breakpoints={{
"(max-width: 539px)": {
  "src": "http://placehold.it/320x160",
  "caption": "For some reason, this is just a captioned image on mobile."
},
"(min-width: 540px)": {
  "caption": "On desktop, this is a looping video. Try resizing the window to see what happens on mobile.",
  "children": <FrameAnimation src='https://video1.nytimes.com/paidpost/belvedere/the-view-from-here/tabletop.mp4' />
}
  }} />

After

Now the multi-line object is correctly indented.

<Responsive as={ThemedFigure}
  title='Shared Figure Title'
  breakpoints={{
    "(max-width: 539px)": {
      "src": "http://placehold.it/320x160",
      "caption": "For some reason, this is just a captioned image on mobile."
    },
    "(min-width: 540px)": {
      "caption": "On desktop, this is a looping video. Try resizing the window to see what happens on mobile.",
      "children": <FrameAnimation src='https://video1.nytimes.com/paidpost/belvedere/the-view-from-here/tabletop.mp4' />
    }
  }} />

@alansouzati
Copy link
Collaborator

Thanks for your contribution. This looks good. Although, I believe the tests should be updated. The CI failed for this reason.

You can run npm run test to see what is failing locally.

This is what is failing in CI:

---
    operator: equal
    expected: |-
      '<Basic test1={function _testCallBack1() {\n    //no-op\n  }} />'
    actual: |-
      '<Basic test1={function _testCallBack1() {\n      //no-op\n    }} />'
    at: Test.<anonymous> (/home/travis/build/grommet/jsx-to-string/test/index.js:172:5)
  ...

@kohlmannj
Copy link
Contributor Author

@alansouzati will fix this week!

@FDiskas
Copy link

FDiskas commented Oct 5, 2017

And.... 🐜

@kohlmannj
Copy link
Contributor Author

Closed in favor of #38.

@kohlmannj kohlmannj closed this Mar 22, 2018
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