Skip to content
This repository has been archived by the owner on Jul 29, 2019. It is now read-only.

Allow disabling of RangeItem limitSize #3050

Merged
merged 6 commits into from
Oct 5, 2017

Conversation

grahamj
Copy link
Contributor

@grahamj grahamj commented May 11, 2017

Some browsers cannot handle very large DIVs so by default range DIVs can be truncated outside the visible area. This change allows the use of a new limitSize item option which disables this functionality, allowing the creation of full-width DIVs.

I don’t see an existing test spec that covers RageItem.js so I’m submitting without tests. However we’ve been using Timeline in production on a fairly large project with these changes in place for several months and it works fine.

I've added the option to the documentation but given that the use case is fairly specific (we're using it so that an SVG graph inserted as a range can line up with the timeline) I'm not sure an example would be helpful.

Some browsers cannot handle very large DIVs so by default range DIVs
can be truncated outside the visible area. This change allows the use
of a new `limitSize` item option which disables this functionality,
allowing the creation of full-width DIVs.

I don’t see an existing test spec that covers RageItem.js so I’m
submitting without tests. However we’ve using Timeline in production on
a fairly large project with these changes in place for several months
and it works fine.
@bradh
Copy link
Contributor

bradh commented May 11, 2017

Can you say which browser causes the problem?

@grahamj
Copy link
Contributor Author

grahamj commented May 12, 2017

@bradh no, and I couldn't find much information on this supposed problem. I do know that full-size DIVs work fine on latest Chrome.

@yotamberk
Copy link
Contributor

@grahamj I don;t understand what this PR fixes. Where did you encounter this issue you mention? How can we test that the issue has been indeed resolved with this PR?

@grahamj
Copy link
Contributor Author

grahamj commented May 16, 2017

@yotamberk this doesn't resolve an issue per se, it allows the disabling of code which was designed to mitigate a supposed browser issue. I didn't write the mitigation code and don't have much information on the supposed issue but I do know that the method it employs to mitigate the issue (limiting the size of range DIVs) makes it very difficult to show content in a range that needs to independently line up with the timeline.

To elucidate: Imagine you have a range item that uses a template to insert a dynamically generated SVG. Inside the SVG you want to draw vertical lines at particular timeline times and all you know is the begin and end times of the range. This is easy if the range's DIV's size is always correlated with the range duration but if its begin or end is arbitrarily cut off there's no way to do it without a lot of hackery.

We're doing this in a production system and I thought the change could be useful for anyone else wanting to do something similar. Plus it's easier for me if I don't have to maintain our own branch :)

@grahamj
Copy link
Contributor Author

grahamj commented May 16, 2017

In case anyone's curious, here's our asset tracking client in action. The temperature and humidity graphs are dynamically generated SVGs. It's an Angular project so I wrapped Timeline as a directive and use range templates to insert the SVG generating directives.

Getting the graph scales in the group DIVs was interesting; would be nice to have group templates as well. One thing at a time ;)

screen shot 2017-05-16 at 11 41 58 am

@yotamberk
Copy link
Contributor

Awesome. I'll review the work and get it in

@grahamj
Copy link
Contributor Author

grahamj commented May 16, 2017

Thank you!

@yotamberk
Copy link
Contributor

After reviewing this I have a bit of a problem accepting this. It seems to me that the better solution is to detect the browser and limit the size automatically when encountering this problem. I know you don't know the specific problematic browsers, but is there a way to detect the problem before it causes the issue? Can you not use this detector and use that to limit the size?

@PhenX
Copy link

PhenX commented May 24, 2017

I think this could be done on initialization:

var e = document.createElement("div");
e.style.width = "10000000000000000000000px";
document.body.appendChild(e);
var max = parseFloat(window.getComputedStyle(e)["width"]);
e.parentNode.removeChild(e);

The max variable will hold the max possible width, if we trust what the browser says.

It looks like most browsers have a max of 33554400px as of this SO page : https://stackoverflow.com/questions/16637530/whats-the-maximum-pixel-value-of-css-width-and-height-properties

@grahamj
Copy link
Contributor Author

grahamj commented May 26, 2017

@PhenX that's interesting about the limits; if Chrome's is 33554428px then we should be hitting it with events longer than about 9.3 hours.

For now, rather than making this more complex and possibly still running into browser limit issues I'm going to see if I can find another way to determine the time extents of a limited DIV on my end.

@grimalschi
Copy link
Contributor

Have same problem. I need large DIVs. Any news?

@yotamberk
Copy link
Contributor

@grahamj What' the status on this PR?

@grahamj
Copy link
Contributor Author

grahamj commented Aug 8, 2017

Oops sorry, I don't check public GH too often. I don't have a better solution. This works for us on Chrome and that's all I really need. I don't have the bandwidth to develop a more general fix and test across platforms.

IMO this is safe as long as devs know about potential issues or it remains undocumented. I guess it's up to you guys where you want to go from here.

@mbroad
Copy link

mbroad commented Sep 5, 2017

Maybe a reasonable compromise would be to choose the lowest known bound as the default max div size, and allow some option to override with a higher range size, or allow users to pass in a calculation function.

Copy link
Contributor

@yotamberk yotamberk left a comment

Choose a reason for hiding this comment

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

Ok, this does do the job and it has been waiting for a while.

@yotamberk yotamberk merged commit e4efee6 into almende:develop Oct 5, 2017
primozs pushed a commit to primozs/vis that referenced this pull request Jan 3, 2019
Some browsers cannot handle very large DIVs so by default range DIVs
can be truncated outside the visible area. This change allows the use
of a new `limitSize` item option which disables this functionality,
allowing the creation of full-width DIVs.

I don’t see an existing test spec that covers RageItem.js so I’m
submitting without tests. However we’ve using Timeline in production on
a fairly large project with these changes in place for several months
and it works fine.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants