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

Show JSX if PropVal is a React element #1455

Closed
wants to merge 4 commits into from

Conversation

sventschui
Copy link

@sventschui sventschui commented Jul 12, 2017

Issue:

What I did

JSX elements within prop values are now rendered completely (in Story Source and Prop Table). The behaviour can be reverted by configuring showSourceOfProps: false.

This yields some errors in the source code generation (with indentation).

I think it would be easier to do the JSX source generation outside of React components since there are a lot of "inverted" dependencies. Rendering the parent depends often on whether the child has multiple lines of code or just a single one.

There is some paddingLeft used for indentation currently what breaks copy & paste behaviour. I think some of the issues outlined could be fixed by using paddingLeft in some cases but I'd rather not do that (because copy & paste).

Would you be open to a diffrent approach on rendering the source code?

image

How to test

Create a story with a property value of type JSX element.

  .addWithInfo('Test', () => {
    const Test = ({ foo, bar, children, arr }) => (<div>
      <div>{foo}</div>
      <div>{bar}</div>
      {arr}
      <div>{children}</div>
    </div>);

    Test.propTypes = {
      foo: PropTypes.node.isRequired,
      bar: PropTypes.node,
      arr: PropTypes.arrayOf(PropTypes.node),
      obj: PropTypes.objectOf(PropTypes.node),
      children: PropTypes.node.isRequired,
    };

    Test.defaultProps = {
      bar: <div>default works in PropTable!</div>,
      arr: null,
      obj: null,
    };

    return (
      <Test foo={<div />} arr={[<Test foo={<div />} />, <Test foo={<div />} />, <Test foo={<div />} />, <Test foo={<div />} />, <Test foo={<div />} />]}>
        <Test
          foo={<div>foo <div>bar</div></div>}
          bar={<div>test</div>}
          arr={[<Test foo={<div />} />, <Test foo={<div />} />, <Test foo={<div />} />, <Test foo={<div />} />, <Test foo={<div />} />]}
          obj={{ foo: <Test foo={<div />} />, bar: <Test foo={<div />} />, }}
        >
          <div>children</div>
        </Test>
      </Test>
    );
  })

@codecov
Copy link

codecov bot commented Jul 12, 2017

Codecov Report

Merging #1455 into master will decrease coverage by 0.32%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1455      +/-   ##
==========================================
- Coverage    14.6%   14.27%   -0.33%     
==========================================
  Files         202      201       -1     
  Lines        4655     4636      -19     
  Branches      528      504      -24     
==========================================
- Hits          680      662      -18     
- Misses       3519     3553      +34     
+ Partials      456      421      -35
Impacted Files Coverage Δ
addons/info/src/index.js 0% <ø> (ø) ⬆️
addons/info/src/components/PropVal.js 0% <0%> (ø) ⬆️
addons/info/src/components/Story.js 0% <0%> (ø) ⬆️
addons/info/src/components/PropTable.js 0% <0%> (ø) ⬆️
addons/info/src/components/Node.js 0% <0%> (ø) ⬆️
addons/info/src/components/Props.js 0% <0%> (ø) ⬆️
app/react/src/client/preview/client_api.js 39.28% <0%> (-2.76%) ⬇️
...codemod/src/transforms/update-organisation-name.js 40.62% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/down_panel.js 23.52% <0%> (ø) ⬆️
addons/knobs/src/components/types/Boolean.js 11.62% <0%> (ø) ⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d63af0d...e444e15. Read the comment docs.

Copy link
Member

@danielduan danielduan left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the PR!

Can you add an example of this in the kitchen sink example app? You should also take a quick glance at the existing info usages there and see that nothing breaks.

@@ -83,9 +87,16 @@ function previewObject(val, maxPropObjectKeys) {
}

export default function PropVal(props) {
const { maxPropObjectKeys, maxPropArrayLength, maxPropStringLength } = props;
const {
braceWrap: _branceWrap,
Copy link
Member

Choose a reason for hiding this comment

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

_braceWrap?

Copy link
Author

Choose a reason for hiding this comment

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

nice catch

@@ -26,7 +26,7 @@ function getData(element) {
}

if (typeof element === 'number') {
data.text = String.toString(element);
data.text = Number.prototype.toString.call(element);
Copy link
Member

Choose a reason for hiding this comment

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

Does this achieve something different than element + ''?

@@ -46,6 +46,7 @@ export default function Node(props) {
const {
node,
depth,
showSourceOfProps,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should get passed as a context instead.

@danielduan
Copy link
Member

To answer your question on the rendering part, I don't think any of us are married to a specific method. A lot of the decisions were made way back when it was hard to predict the complexity of info.

If you think there's a more logical way to render it, we'd love a PR.

@sventschui
Copy link
Author

I'll try to elaborate on a more robust way to render the source. I think the copy & paste use case is big enough issue for me to stop using padding-left as indentation.

I'll add an example to the kitchen sink. Thanks for the heads up.

@ndelangen
Copy link
Member

@sventschui could you have a look at the merge conflicts? 🙇

@sventschui
Copy link
Author

@ndelangen I think you can close this PR. This will yield some errors. The source code generation thingy needs to be refactored

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

Successfully merging this pull request may close these issues.

None yet

4 participants