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

Add ability to parse line breaks into <br/> for Docgen description in… #2053

Merged

Conversation

dangreenisrael
Copy link
Member

@dangreenisrael dangreenisrael commented Oct 15, 2017

Issue: #1311

What I did

Added automatic parsing of line breaks into <br/> for the Docgen descriptions. I opted for this route over full blown parsing markdown to prevent any weird formatting issues with the table.

How to test

Is this testable with jest or storyshots?

This is fully tested with jest

Does this need a new example in the kitchen sink apps?

The Docgen button was updated

Does this need an update to the documentation?
No

screen shot 2017-10-15 at 0 38 02

screen shot 2017-10-14 at 23 58 52

// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`PropTable multiLineText should have 2 br tags for 3 lines of text 1`] = `
Array [
Copy link
Member Author

Choose a reason for hiding this comment

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

ESlint rules seem to prevent these from being one line each

Copy link
Member

Choose a reason for hiding this comment

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

Eslint shouldn't do anything with js.snap files. Please check your editor settings

Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually this that ESlint doesn't want me to make a 1 liner. https://github.com/storybooks/storybook/pull/2053/files#diff-93c8453ec93a245a8bc84801b42540c7R96. That is causing this snapshot to be so big (not that that really matters)

@dangreenisrael dangreenisrael force-pushed the addon-info-add-ability-new-line-in-description-#1311 branch from 1fe6754 to 6273258 Compare October 15, 2017 15:51
@codecov
Copy link

codecov bot commented Oct 15, 2017

Codecov Report

Merging #2053 into release/3.3 will increase coverage by 0.03%.
The diff coverage is 50%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.3    #2053      +/-   ##
===============================================
+ Coverage         22.3%   22.33%   +0.03%     
===============================================
  Files              326      326              
  Lines             6537     6555      +18     
  Branches           810      829      +19     
===============================================
+ Hits              1458     1464       +6     
+ Misses            4476     4474       -2     
- Partials           603      617      +14
Impacted Files Coverage Δ
addons/info/src/components/PropTable.js 23.93% <50%> (+1.7%) ⬆️
app/vue/src/server/config/babel.js 0% <0%> (-100%) ⬇️
lib/ui/src/modules/ui/containers/stories_panel.js 25.71% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/addon_panel.js 23.52% <0%> (ø) ⬆️
app/react/src/client/preview/element_check.js 41.17% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_keyevents.js 33.33% <0%> (ø) ⬆️
addons/links/src/components/link.js 16.66% <0%> (ø) ⬆️
lib/ui/src/modules/api/configs/init_api.js 40.42% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/hierarchy.js 50.74% <0%> (ø) ⬆️
... and 38 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 5c3fc6c...5524aac. Read the comment docs.

@dangreenisrael dangreenisrael requested a review from a team October 15, 2017 16:04
@@ -82,6 +82,22 @@ const propsFromPropTypes = type => {
return props;
};

export const multiLineText = text => {
if (!text) return '';
Copy link
Member Author

Choose a reason for hiding this comment

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

Does anyone have a preference here to return null and therefore render <td/> versus an empty string('') and <td></td>

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 I'd prefer the short version

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@@ -82,6 +82,22 @@ const propsFromPropTypes = type => {
return props;
};

export const multiLineText = text => {
if (!text) return '';
const arrayOfText = text.replace(/\r?\n|\r/g, '--newline--').split('--newline--');
Copy link
Member

Choose a reason for hiding this comment

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

Why not just split on global regexp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@Hypnosphi
Copy link
Member

Hypnosphi commented Oct 16, 2017

Please update test snapshot and merge as soon as CI jobs pass

@dangreenisrael
Copy link
Member Author

Will do when I get home tonight

@Hypnosphi Hypnosphi merged commit a3cb6fb into release/3.3 Oct 16, 2017
@Hypnosphi Hypnosphi deleted the addon-info-add-ability-new-line-in-description-#1311 branch October 16, 2017 23:54
@ndelangen
Copy link
Member

Awesome, great work @dangreenisrael !! 💪

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

3 participants