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(demo/text): render 0 as number (#813) #814

Merged
merged 4 commits into from
Oct 8, 2020

Conversation

ilariaventurini
Copy link
Contributor

@ilariaventurini ilariaventurini commented Sep 26, 2020

@coveralls
Copy link

coveralls commented Sep 26, 2020

Pull Request Test Coverage Report for Build 138

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 57.448%

Totals Coverage Status
Change from base Build 134: 0.0%
Covered Lines: 1285
Relevant Lines: 2172

💛 - Coveralls

Copy link
Member

@hshoff hshoff 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 contribution! Looks good to me.

@williaster
Copy link
Collaborator

@ilariaventurini thanks for the fix!

looks like the build is failing due to eslint errors. The seem to all be auto-fixable so if you run yarn lint --fix from the monorepo root it should fix everything ✨

@ilariaventurini
Copy link
Contributor Author

ilariaventurini commented Sep 30, 2020

I edited the code using props.children !== null (as @williaster suggested me) instead of props.children !== null && props.children !== undefined.
Tests all pass.
Then I tried running yarn fix:lint but I get this error:

/Users/ilariaventurini/.../visx/packages/visx-text/src/Text.tsx
119:11  error  Unexpected negated condition  no-negated-condition
141:7   error  Unexpected negated condition  no-negated-condition

So the build is failing.

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

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

Hey @ilariaventurini , sorry for the misdirection in suggestiong eslint --fix, it looks like that converted a lot of TypeScript anys to unknown which aren't equivalent, and now failed the build.

I think it's probably easiest to revert back to your first commit 9f98551, and then apply my suggested fix to reverse the negated condition. Then this should be good to go 🙏

@@ -138,7 +138,7 @@ class Text extends React.Component<TextProps, TextState> {
}

updateWordsWithoutCalculate(props: TextProps) {
const words = props.children ? props.children.toString().split(/(?:(?!\u00A0+)\s+)/) : [];
const words = (props.children !== null && props.children !== undefined) ? props.children.toString().split(/(?:(?!\u00A0+)\s+)/) : [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

!= I think is still ideal, but there is another lint rule which favors not having negated conditions (because they are harder to understand logically). So I think this should fix things:

Suggested change
const words = (props.children !== null && props.children !== undefined) ? props.children.toString().split(/(?:(?!\u00A0+)\s+)/) : [];
const words = props.children == null ? [] : props.children.toString().split(/(?:(?!\u00A0+)\s+)/);

@williaster
Copy link
Collaborator

@ilariaventurini sorry looks like you still have all the auto lint fix stuff in here. when I said revert back to your first commit I meant run git reset --hard 9f98551.

That commit had lint issues, so from there you should simply be able to make the change to const words = props.children == null ? [] : props.children.toString().split(/(?:(?!\u00A0+)\s+)/) then force push your branch.

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

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

Thanks @ilariaventurini !

@williaster williaster merged commit 5f23351 into airbnb:master Oct 8, 2020
@williaster williaster added this to the 1.1.0 milestone Oct 17, 2020
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.

@vx/text doesn't work with 0 as a number
4 participants