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

Addon-docs: Preserve Source indentation by default #9513

Merged
merged 3 commits into from
Jan 18, 2020

Conversation

atanasster
Copy link
Member

Issue: #8078

What I did

  • Removed default decent (remove template strings spacing)
  • code styling prevent white space wrapping style

How to test

I didn't get much stories to try with, so mostly tested indentation.
Possibly find stories that are using template strings?

@vercel
Copy link

vercel bot commented Jan 17, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/e2sp097bq
✅ Preview: https://monorepo-git-fork-atanasster-code-preview-remove-dedent.storybook.now.sh

@shilman shilman changed the title remove dedent default and white space wrapping SyntaxHighlighter: Remove dedent default and white space wrapping Jan 18, 2020
@shilman
Copy link
Member

shilman commented Jan 18, 2020

This is "kind of" a breaking change. I think it's probably better than what it's replacing in most cases. Here's a case where it's not:

Addons___Actions_-_Multiple_actions_⋅_Storybook

Addons___Actions_-_Multiple_actions_as_object_⋅_Storybook_and_Storybook_Roadmap_Management_-_Google_Docs

@shilman
Copy link
Member

shilman commented Jan 18, 2020

@atanasster Oh, see also the failing chromatic tests! 😱

@atanasster
Copy link
Member Author

This is "kind of" a breaking change. I think it's probably better than what it's replacing in most cases. Here's a case where it's not:

Can you please elaborate, the new version is outputting it as it is in the code, while before it was mangling the code> no ?

Here is the code for the story:

export const MultipleActionsObjectConfig = () => (
  <Button
    {...actions({ onClick: 'clicked', onMouseOver: 'hovered' }, { clearOnStoryChange: false })}
  >
    Moving away from this story will persist the action logger
  </Button>
);
``

@atanasster
Copy link
Member Author

chromatic tests - no idea what they say :)
Like one issue seems to be a styled-components className? And some issues are expectedly the code output changes?

@atanasster
Copy link
Member Author

atanasster commented Jan 18, 2020

Can you please elaborate, the new version is outputting it as it is in the code, while before it was mangling the code> no ?

If the issue is the long line, I specifically removed the white-space wrapping style, so the code stays the same as the story was written and the user has control over the code. I can put it back in (to force-wrap lines)

@shilman
Copy link
Member

shilman commented Jan 18, 2020

I took a look at all the different permutations. What I think:

  1. Leave the SourceHighlighter alone
  2. Modify format=false as the default on the lib/components Source component instead

That way we still get line wrapping (debatable, I know) but most of the annoying bugs that people are reporting go away. WDYT?

@atanasster
Copy link
Member Author

sounds good, checking in

@vercel vercel bot temporarily deployed to Preview January 18, 2020 07:03 Inactive
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM 😅

@shilman shilman changed the title SyntaxHighlighter: Remove dedent default and white space wrapping Addon-docs: Preserve Source indentation by default Jan 18, 2020
@shilman shilman merged commit 93afd7f into storybookjs:next Jan 18, 2020
@shilman shilman deleted the code-preview-remove-dedent branch January 18, 2020 10:18
@patricklafrance
Copy link
Member

patricklafrance commented Jan 22, 2020

Not sure if these changes are a good idea.

Before 5.3.7 about 15% of my source were not formatted well. Now it's about 90%.

Most of them are like the following:

image

@shilman
Copy link
Member

shilman commented Jan 23, 2020

@atanasster WDYT

@atanasster
Copy link
Member Author

@patricklafrance - can you paste your code that breaks here?

@shilman yes, lets keep for the 6.0 branch, its too much of a risk to push out. In general, i think we should not touch the source code on our end (including in source-loader) and use the syntax highlighter’s prettier configuration that would be also available to the end-user - ie he prefers 2 vs 4 spaces etc.

@patricklafrance
Copy link
Member

@atanasster for this particular case:

<Preview>
    <Story name="size">
        <div className="flex flex-row items-end">
            <Button type="button" size="tiny">Tiny</Button>
            <Button type="button" size="small">Small</Button>
            <Button type="button" size="medium">Medium</Button>
            <Button type="button" size="large">Large</Button>
        </div>
    </Story>
</Preview>

I have a few other cases:

image

<Preview>
    <Story name="default">
        <RemoteSearchInput
            onFetchResults={useDefaultResultsFetcher("https://wft-geo-db.p.rapidapi.com/v1/geo/countries?limit=5", "namePrefix", { requestOptions: API_REQUEST_OPTIONS })}
            onResults={response => {
                return response.data.map(x => searchInputResult(x.code, x.name));
            }}
            onValueChange={logValueChanged}
            placeholder="Type a country name"
        />
    </Story>
</Preview>

image

<Preview>
    <Story name="disabled">
        <MultiSelect
            items={DEFAULT_ITEMS}
            defaultValues={[GROUP_RESTORED_VALUE, GROUP_NAME_CHANGED_VALUE]}
            disabled
            onValuesChange={logValuesChanged}
        />
    </Story>
</Preview>

image

<Preview>
    <Story name="custom-color">
        <Label
            naked
            style={{
                backgroundColor: "#FFF6E7",
                color: "var(--marine-500)",
                border: "1px solid #FEE9C3"
            }}
        >
            Notification Sent
        </Label>
    </Story>
</Preview>

@atanasster
Copy link
Member Author

@patricklafrance thanks !

@shilman
Copy link
Member

shilman commented Jan 23, 2020

@atanasster This has already been released in 5.3.7. Should I revert the change there?

@atanasster
Copy link
Member Author

Ah sorry didnt realize. Based on what @patricklafrance is saying, i guess we should revert it if possible?

@shilman
Copy link
Member

shilman commented Jan 23, 2020

👍 will revert it @patricklafrance @atanasster and let's figure out the right fix on the next/6.0 branch

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

Successfully merging this pull request may close these issues.

3 participants