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

Fix Property types don't show up #1563

Merged
merged 4 commits into from Mar 17, 2020
Merged

Conversation

mitsuruog
Copy link
Contributor

Resolve

I organize react-docgen prop type(Named PropDescriptor) to have flowType and tsType both and I remove "WithFlow" word from the types.

review me please.

Default: Required
Description:"
`);
describe('flowType', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the tests for the flowType by wrapping describe

@codecov
Copy link

codecov bot commented Mar 16, 2020

Codecov Report

Merging #1563 into master will decrease coverage by 0.02%.
The diff coverage is 96.42%.

Impacted Files Coverage Δ
src/client/rsg-components/Usage/Usage.tsx 100% <ø> (ø) ⬆️
src/client/rsg-components/Props/renderDefault.tsx 100% <100%> (ø) ⬆️
src/client/rsg-components/Props/renderExtra.tsx 91.66% <100%> (ø) ⬆️
src/client/rsg-components/Props/util.ts 100% <100%> (ø) ⬆️
src/client/rsg-components/Props/PropsRenderer.tsx 100% <100%> (ø) ⬆️
src/client/rsg-components/Props/renderType.tsx 93.61% <91.66%> (-0.98%) ⬇️

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request!

) {
const props = parse(
`
// @TypeScript
Copy link
Member

Choose a reason for hiding this comment

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

This comment doesn't mean anything in TypeScript and will confuse readers.

});
});

describe('TypeScript', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any difference between Flow and TypeScript test cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is some different output. and I also added for the unknown type for the TypeScript.

Copy link
Member

Choose a reason for hiding this comment

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

But the input is the same for all but unknown? Can we use describe.each here to merge them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. I didn't have this idea. Let me try once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sorry, the input has a bit different.
I made to be able to inject preparatory code into test components for TypeScript.
I probably can merge them, do you still want me to?

Copy link
Member

Choose a reason for hiding this comment

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

Could you show an example? I believe we want to test very similar things, so merging makes sense. But maybe the difference is too big? ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/styleguidist/react-styleguidist/pull/1563/files#diff-ea72f02dad8b44122fd2deb5537e1b1bR703

Only here is different. the difference is not too big, so I probably can merge them.

src/client/rsg-components/Props/util.ts Show resolved Hide resolved
[
'flowType',
renderFlow,
{ enum: { declaration: "type MyEnum = 'One' | 'Two'", expect: { type: 'enum' } } },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use the same renderFlowType (Now the name has changed to renderAdvancedType) for each.
Regarding rendering result, only different is the part of "enum". other types have the same result.

@mitsuruog
Copy link
Contributor Author

@sapegin please review me again.

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

@sapegin sapegin merged commit f7f06f9 into styleguidist:master Mar 17, 2020
@styleguidist-bot
Copy link
Collaborator

🎉 This PR is included in version 11.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mitsuruog mitsuruog deleted the fix-1551 branch March 17, 2020 13:09
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.

None yet

3 participants