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

enable truncation based on single long words and number of line restriction #91

Merged
merged 10 commits into from
Jun 5, 2018

Conversation

IPWright83
Copy link

@IPWright83 IPWright83 commented May 31, 2018

Added a maximum number of lines feature: closes #75
Resolved issues where no text was being displayed when a single word was too large: closes #76

Description

For #75 I've added an additional maxLines() option that can be changed on TextBox.js. This defaults to null which indicates there is no limit applied. When wrapping occurs this gets passed down into textWrap.js and is considered when it determines whether truncation needs to occur.

For #76 this was due to the word being too long to physically fit, therefore lineData was being set to an empty array yielding a blank result. I've modified this to call the ellipsis function and pass the result into a new textTruncate function. This will reduce the length of the original word and append ellipsis until it can successfully fit in the given width constraints. This does change the behavior for a single long word so have indicated as a breaking change.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have documented all new methods.
  • I have written tests for all new methods/functionality.
  • I have written examples for all new methods/functionality.

Copy link
Member

@davelandry davelandry left a comment

Choose a reason for hiding this comment

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

great work @IPWright83! I left a couple comments. After that, this should be good to go.

src/textWrap.js Outdated
@@ -125,6 +126,15 @@ export default function() {
return arguments.length ? (lineHeight = _, textWrap) : lineHeight;
};

/**
@memberof textWrap
@desc If *value* is specified, sets the maximum number of lines allowed when wrapping
Copy link
Member

Choose a reason for hiding this comment

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

please add a period to the end of this sentence

src/TextBox.js Outdated
else if (fS > fMax) fS = fMax;
// Constraint the font size
fS = Math.max(fS, fMin);
fS = Math.min(fS, fMax);
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to use the max and min functions imported from d3-array instead of the vanilla JS Math functions.

Copy link
Author

Choose a reason for hiding this comment

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

Interested to know the reason for this? Happy to make the change but I'm always pro vanilla if possible

Copy link
Member

Choose a reason for hiding this comment

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

it's just ever-so-slightly more robust than the built-in (and is already included as a dependency). I suppose in this case it doesn't matter ¯\(ツ)

Unlike the built-in Math.max, this method ignores undefined values; this is useful for ignoring missing data. In addition, elements are compared using natural order rather than numeric order. For example, the maximum of the strings [“20”, “3”] is “3”, while the maximum of the numbers [20, 3] is 20.


d3PlusWrap(g, circles);

</script>
Copy link
Member

Choose a reason for hiding this comment

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

please move this file to a directory called dev/, which will then remove it from the repo. We don't store test HTML files in the repository.

@IPWright83
Copy link
Author

@davelandry Thanks for your feedback. I'll look at making those changes.

One thing of note is that I'm struggling a little getting it working within our product at the moment (you might be able to help). I'm including via an npm install, and getting issues with all the d3 module imports being undefined.

Have you come across anything similar?

@IPWright83
Copy link
Author

@davelandry let me know if this is ok. If so be great if you have a chance to publish before Monday. Save me publishing my own internal version to use :)

@davelandry davelandry changed the title Enable truncation based on single long words and number of line restriction enable truncation based on single long words and number of line restriction Jun 5, 2018
@davelandry davelandry merged commit c75ee06 into d3plus:master Jun 5, 2018
@davelandry
Copy link
Member

@IPWright83 just published this PR with a new release. thanks again!

what kind of errors are you getting with installing using npm?

@davelandry
Copy link
Member

also, what type of environment are you installing it into?

@IPWright83
Copy link
Author

@davelandry I think I'm managed to fix my npm issues now fortunately. Took a bit of battling but we have our own d3 bundle (due to an old npm bug) and essentially I was getting 2 instances of d3.selection - which meant the prototype hadn't been extended to include .transition etc. All good now though :)

Glad to have helped, thanks for producing the library in the first place.

@anatolyg
Copy link

anatolyg commented Jun 7, 2018

@IPWright83 can you publish the solution to your NPM troubles? We're having the same exact issue, also due to the extra addition of d3. Getting the selection prototype not having a transition function. Thank you!

@IPWright83
Copy link
Author

@anatolyg Took me a while to sort out, but I was getting the following structure (bearing in mind we have our own bundle of d3):

|node_modules
|---- companyx/d3
|---- d3plus-text
|-------- node_modules
|------------ d3-selection
|---- d3-selection

Note that the d3-selection was sometimes under companyx/d3 too and not d3plus-text. So in the package.json files I had:

companyx/d3
d3-selection: ^1.1.0

d3plus-text
d3-selection: ^1.3.0

So while it should have been able to pull in just 1.3.0 or above, it didn't seem to be doing so. I deleted the packagelock.json a few times which didn't help much. In the end I upped the companyx/d3 version to d3-selection: ^1.3.0, also added d3-selection to my main package.json using ^1.3.0, ran a fresh npm install and an npm dedupe and that seemed to get rid of the extra references.

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.

no text appearing in subsequent renders on Windows Max number of lines
3 participants