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 Wrong union description with TypeScript #1621 #1629

Merged
merged 7 commits into from Aug 24, 2020

Conversation

@mitsuruog
Copy link
Contributor

@mitsuruog mitsuruog commented Jul 12, 2020

Resolved:

  • Wrong union description with TypeScript #1621

Main changes:

  • Create a new component named ComplexType and Tooltip to show the complex type
  • Introduce tippyjs-react in the project
  • Add styleMock in the test assets to mock CSS import

Hover Actions:
Jul-12-2020 22-42-08

Keyboard Actions:
(use Tab key)
Jul-12-2020 22-42-58

package.json Outdated Show resolved Hide resolved
@mitsuruog
Copy link
Contributor Author

@mitsuruog mitsuruog commented Jul 14, 2020

I can't pass the CI steps of npm run build:sections due to the next error.

asset size limit: The following asset(s) exceed the recommended size limit (1.05 MiB).

@sapegin
Can I remove the performance settings here?
https://github.com/styleguidist/react-styleguidist/blob/master/examples/sections/styleguide.config.js#L92-L99

Copy link
Member

@sapegin sapegin left a comment

Thanks for the pull request! This is a very useful feature. I've left a few general comments on the implementation but this is a very good start!

About the size checker: how much bigger it is now? It is here to avoid an accidental increase of the bundle size.

@mitsuruog
Copy link
Contributor Author

@mitsuruog mitsuruog commented Jul 14, 2020

Actually the bundle size is increased 1.1MB => 1.5MB. In my opinion, this increase is unacceptable.
However, it is almost close to the limit as it is now, and it may have the same problem again in the near future.

@sapegin
Copy link
Member

@sapegin sapegin commented Jul 14, 2020

That's the idea: the limit should be very close to the actual size so any increase is visible. Let's check again with custom styles, I believe they'll be much smaller.

@mitsuruog mitsuruog force-pushed the mitsuruog:fix-1621 branch from 742c059 to 1e6a574 Jul 23, 2020
@mitsuruog
Copy link
Contributor Author

@mitsuruog mitsuruog commented Jul 23, 2020

@sapegin Hi, I have updated my PR but I am still facing the CI step problem.
Actually the bundle size is increased 1.1MB => 1.11MB not so bad.

@sapegin
Copy link
Member

@sapegin sapegin commented Jul 23, 2020

Have you tried to update the limit?

@mitsuruog
Copy link
Contributor Author

@mitsuruog mitsuruog commented Jul 23, 2020

not yet. I can update it, but what do you think the next limit should be?

@codecov
Copy link

@codecov codecov bot commented Jul 23, 2020

Codecov Report

Merging #1629 into master will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
...sg-components/ComplexType/ComplexTypeRenderder.tsx 100.00% <100.00%> (ø)
src/client/rsg-components/Props/renderType.tsx 96.96% <100.00%> (+7.78%) ⬆️
.../client/rsg-components/Tooltip/TooltipRenderer.tsx 100.00% <100.00%> (ø)
@sapegin
Copy link
Member

@sapegin sapegin commented Jul 23, 2020

As I've already explained: as close as possible, so 1.15 should be fine.

@mitsuruog
Copy link
Contributor Author

@mitsuruog mitsuruog commented Jul 24, 2020

hmmm... I have no idea how to increase the coverage any further.

@sapegin
Copy link
Member

@sapegin sapegin commented Jul 24, 2020

I believe covering this line is the way:

return <Type>unknown</Type>;
@mitsuruog
Copy link
Contributor Author

@mitsuruog mitsuruog commented Jul 24, 2020

Do you know what input to give to reach this case?

function renderAdvancedType(type: TypeDescriptor): React.ReactNode {
if (!type) {

This comment has been minimized.

@mitsuruog

mitsuruog Jul 27, 2020
Author Contributor

@sapegin
I think the line above is usually unreachable code for now.

Because:

  • The type passed to this function is based on prop.flowType or prop.tsType.
  • This function does not enter the branch unless prop.flowType or prop.tsType is defined.

Therefore, what I can do is:

  • Remove unreachable codes to increase coverage.
  • Test only this function by itself to get the coverage
  • Allow for this reduction in coverage.

what do you think?

I'd like to keep this line and test it on its own, as there may be some changes in the future that might unintentionally go into this branch. in my opinion tho.

This comment has been minimized.

@sapegin

sapegin Jul 27, 2020
Member

I think removing makes more sense — according to code and types it should be defined. And it would be great to fix type as any on the call site.

This comment has been minimized.

@mitsuruog

mitsuruog Jul 27, 2020
Author Contributor

And it would be great to fix type as any on the call site.

good point. haha.
but It may be difficult in places where the type definitions are a bit mixed up.
I'll try it out for a bit, but if it's difficult, I think I'll ask for a review as it is.

This comment has been minimized.

@sapegin

sapegin Jul 27, 2020
Member

Thanks! Don't spend too much time on it though ;-)

@@ -58,7 +58,6 @@ declare module 'react-docgen' {
| 'objectOf'
| 'shape'
| 'exact'
| 'union'

This comment has been minimized.

@mitsuruog

mitsuruog Jul 27, 2020
Author Contributor

duplicated definition for TypeUnionDescriptor

@mitsuruog
Copy link
Contributor Author

@mitsuruog mitsuruog commented Jul 27, 2020

@sapegin please review me again when you have time!

Copy link
Member

@sapegin sapegin left a comment

Very good, we're getting there! Just a few minor things left. 🦄

expect(container).toContainElement(getByTestId('child'));
});

test('should the child component be wrapped by "role=button" element', () => {

This comment has been minimized.

@sapegin

sapegin Jul 30, 2020
Member

Testing that this element is clickable and actually doing something (opening a tooltip) would be more useful, this test doesn't add value to other tests.

This comment has been minimized.

@mitsuruog

mitsuruog Jul 30, 2020
Author Contributor

it's fine to remove it as it's been tested in other cases.

await waitFor(() =>
expect(container.querySelector('[data-state="visible"]')).toBeInTheDocument()
);
expect(container.querySelector('[data-state]')).toHaveAttribute('data-state', 'visible');

This comment has been minimized.

@sapegin

sapegin Jul 30, 2020
Member

This line is the same assertion as the previous, so you can drop it.

export const styles = ({ space }: Rsg.Theme) => ({
complexType: {
alignItems: 'center',
cursor: 'pointer',

This comment has been minimized.

@sapegin

sapegin Jul 30, 2020
Member

Shouldn't this be in the Tooltip component?

This comment has been minimized.

@mitsuruog

mitsuruog Jul 30, 2020
Author Contributor

I moved it to the tooltip, but the cursor didn't change as I expected, so I added a style here.
However, I think this cursor should change depending on the component it's wrapped around, so I would like to remove it for now.

alignItems: 'center',
cursor: 'pointer',
display: 'inline-flex',
'& span': {

This comment has been minimized.

@sapegin

sapegin Jul 30, 2020
Member

What's this span? Looks like this selector is very fragile and relies on the implementation of another component.

This comment has been minimized.

@mitsuruog

mitsuruog Jul 30, 2020
Author Contributor

Good point. this span is actually the icon. but I should have used a more safe way to avoid breaking.

@mitsuruog
Copy link
Contributor Author

@mitsuruog mitsuruog commented Jul 30, 2020

@sapegin please review me again.

@mitsuruog mitsuruog force-pushed the mitsuruog:fix-1621 branch from cd9f72c to 444f622 Aug 24, 2020
@mitsuruog
Copy link
Contributor Author

@mitsuruog mitsuruog commented Aug 24, 2020

@sapegin review me, please.

@sapegin sapegin merged commit a9783d6 into styleguidist:master Aug 24, 2020
10 checks passed
10 checks passed
Header rules - styleguidist No header rules processed
Details
Pages changed - styleguidist All files already uploaded
Details
Redirect rules - styleguidist No redirect rules processed
Details
Danger All green. Yay.
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
Mixed content - styleguidist No mixed content detected
Details
codecov/patch 100.00% of diff hit (target 97.18%)
Details
codecov/project 97.35% (+0.16%) compared to 53bc4bb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/styleguidist/deploy-preview Deploy preview ready!
Details
@sapegin
Copy link
Member

@sapegin sapegin commented Aug 24, 2020

Thanks, merging! Sorry for the delay ;-/

@styleguidist-bot
Copy link
Collaborator

@styleguidist-bot styleguidist-bot commented Sep 2, 2020

🎉 This PR is included in version 11.0.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.