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

allow height/contentHeight to accept a function for dynamic value #3271

Merged
merged 10 commits into from
Jul 31, 2016

Conversation

caseyjhol
Copy link
Contributor

This is a fork of #3040. Instead of adding a new getContainerHeight option, this simply allows height and contentHeight to accept a function.

See a preview here: http://plnkr.co/edit/O0iBHMcFa0faPE8XyPb3?p=preview where height is a function that returns the height of the calendar's parent (which is an absolutely positioned DIV with {top: 200px, right: 0, bottom: 0, left: 0;}).

aharpervc and others added 5 commits February 9, 2016 15:39
this makes it a lot easier to size the control to its container. for example, implementations could do `return container_el.clientHeight`
@@ -108,6 +114,61 @@
});
});

describe('as a function, when there are no events', function() {
Copy link
Member

Choose a reason for hiding this comment

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

not a big deal, but this is a lot of copypasta from the other tests. would you be able to DRY it up? recommended format to do so:

[
    { description: 'as a number', height: 600 },
    { description: 'as a function', height: getParentHeight, heightWrapper: true }
].forEach(function(testInfo) {

    describe(testInfo.description, function() {

        if (testInfo.heightWrapper) {
            beforeEach(function() {
                calendarEl.wrap('<div class="calendar-container" style="height: 600px;" />');
            });
        }

        describe('when there are no events', function() {
            // ...
        }); 
        // ...
    });
});

Copy link
Member

Choose a reason for hiding this comment

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

also, this will make a "parent" setting much easier to write tests for (pretty much the same tests as getParentHeight)

@arshaw
Copy link
Member

arshaw commented Jul 12, 2016

awesome! thanks for carrying this through to the finish line. would you be able to add another setting as a shortcut to setting a 100% to the parent? the value would just be 'parent'.

One thing we might want to test for is that the function and 'parent' args get updated upon window resize. Making an automated test for this will be too cumbersome and inaccurate. But would you mind making a manual test for this?

Could you just modify this existing manual test?
https://github.com/fullcalendar/fullcalendar/blob/master/tests/fullheight.html
Will be a great proof of concept!

@arshaw arshaw added the WIP label Jul 12, 2016
@arshaw
Copy link
Member

arshaw commented Jul 13, 2016

looks great! thanks for the hard work on this one. i'll merge it into the upcoming release.

@arshaw arshaw added Mergeable and removed WIP labels Jul 13, 2016
@caseyjhol
Copy link
Contributor Author

caseyjhol commented Jul 14, 2016

In my testing, the height does get auto-updated when the window is resized, but not as smoothly as when doing $(window).resize (i.e. it updates less frequently). It looks like that's because windowResizeDelay is set to 200ms. I can manually override that to 0, but I didn't see that in the documentation.

@arshaw
Copy link
Member

arshaw commented Jul 14, 2016

I'll document windowResizeDelay also make the default value significantly smaller

@arshaw arshaw added this to the v2.9.1 milestone Jul 30, 2016
@arshaw arshaw merged commit 6e6e42c into fullcalendar:master Jul 31, 2016
@arshaw
Copy link
Member

arshaw commented Aug 1, 2016

@aharpervc
Copy link
Contributor

cool 👍

@caseyjhol caseyjhol deleted the get-container-height-option branch November 2, 2017 19:52
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.

3 participants