Skip to content
This repository has been archived by the owner on Oct 28, 2020. It is now read-only.

Use spaces option if boolean is provided #98

Merged
merged 4 commits into from
Jun 27, 2018

Conversation

mattsacks
Copy link
Contributor

When setting the option spaces to false, it would be ignored due to the default being true.

This PR checks to see if a boolean option is provided for spaces and uses that before defaulting to true.

@mattsacks mattsacks changed the title Use spaces option is boolean is provided Use spaces option if boolean is provided Jun 25, 2018
@yowainwright
Copy link
Contributor

I might be missing something...but the sentence provided above and the pr logic are incorrect.

Thanks for taking the time to review the code and submit a PR!

@mattsacks
Copy link
Contributor Author

mattsacks commented Jun 25, 2018

Hey @yowainwright, you're right! I goofed on the ternary syntax 🤦‍♂️

As of now, if opts.spaces is set to false (or any falsy value), it would always be ignored even if we wanted spaces to be false. With the provided fix, if opts.spaces is a boolean, it'll respect whatever value was set. Otherwise, it'll default to true.

Let me know if you'd like me to re-submit the PR with the updated code.

@yowainwright
Copy link
Contributor

The code that is there is tested and appears to be working as expected.

If you would like to take the perceived issue to code editing tool, here's a codepen with spaces set to false.

@mattsacks
Copy link
Contributor Author

@yowainwright yep! So there, it looks like it's truncating within the word whereas the desired behavior by setting opts.spaces = false would be to split between words. When I changed line 82 between false and true, I didn't actually see any difference rendered (the original issue my PR attempts to fix).

If you check out this codepen with the applied fix, you can see that switching between spaces: false and spaces: true will change where truncation happens (i.e, after the word food and within the word truck respectively).

@yowainwright
Copy link
Contributor

yowainwright commented Jun 25, 2018

shave__a_javascript_plugin_for_truncating_text_-_non-spaced_language_support

In the Codepen example that was shared above, non-spaced languages are no longer getting truncated.

@mattsacks
Copy link
Contributor Author

@yowainwright Yep! That's because there's nothing in the code to .split(' ') on (called when opts.spaces === false). So I actually think that's working as intended there?

@yowainwright
Copy link
Contributor

yowainwright commented Jun 27, 2018

@mattsacks darn it! Thank you for taking the time to continually explain this issue.

You are 100% right! This issue was introduced a while ago too. 😩

Do you still want to provide the pr/fix?

@yowainwright yowainwright reopened this Jun 27, 2018
@mattsacks
Copy link
Contributor Author

@yowainwright I’d love to!

By re-opening this PR, it looks like it got my update for the proper ternary syntax. Is there anything else you need before this can be merged?

@yowainwright
Copy link
Contributor

yowainwright commented Jun 27, 2018

@mattsacks if you'd like to provide a patch version bump in package.json to 2.2.1 and then build (npm run build), you're more than welcome to!

Other wise, this is prefect. Thanks again! ...And sorry it took, me a bit see the bug.

@mattsacks
Copy link
Contributor Author

@yowainwright more than happy to, I’ll get that up for you in just a bit.

And no prob! I’m glad we’re able to get this fixed 🙏

@mattsacks
Copy link
Contributor Author

@yowainwright all set!

Copy link
Contributor

@yowainwright yowainwright left a comment

Choose a reason for hiding this comment

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

🚢

@yowainwright yowainwright merged commit cacbc9d into dollarshaveclub:master Jun 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants