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

Added fixedWeeks flag #176

Merged
merged 1 commit into from Jun 8, 2016

Conversation

3 participants
@fcsonline
Contributor

fcsonline commented Jun 6, 2016

Problem

If your calendar is in a container with other elements around and you move between months, the container resizes to the number of weeks shown and this can create an annoying effect.

Solution

Added fixedWeeks flag

With this new flag, the calendar will show always the same number of
weeks. Exactly 6, because is the maximum number of week that a month can
have.

You can see here an example with this flag enabled:

6weeks

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jun 6, 2016

Owner

👋 @fcsonline

I guess this is a dup of #133 (see comment). Maybe I don't get it: what is the problem with setting the container's min-height?

Owner

gpbl commented Jun 6, 2016

👋 @fcsonline

I guess this is a dup of #133 (see comment). Maybe I don't get it: what is the problem with setting the container's min-height?

@fcsonline

This comment has been minimized.

Show comment
Hide comment
@fcsonline

fcsonline Jun 6, 2016

Contributor

The problem is that for months with 4 or 5 weeks you'll see a weird empty space. With this you can ensure that the calendar will show data for them.

Contributor

fcsonline commented Jun 6, 2016

The problem is that for months with 4 or 5 weeks you'll see a weird empty space. With this you can ensure that the calendar will show data for them.

@fcsonline

This comment has been minimized.

Show comment
Hide comment
@fcsonline

fcsonline Jun 6, 2016

Contributor

I'm going to fix those eslint issues.

Contributor

fcsonline commented Jun 6, 2016

I'm going to fix those eslint issues.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jun 6, 2016

Owner

However, for months having just four weeks, you would see two weeks more from the next month, which is also weird. I think such graphic glitches should not be handled by this component. How are other implementations handling with this?

Owner

gpbl commented Jun 6, 2016

However, for months having just four weeks, you would see two weeks more from the next month, which is also weird. I think such graphic glitches should not be handled by this component. How are other implementations handling with this?

@fcsonline

This comment has been minimized.

Show comment
Hide comment
@fcsonline

fcsonline Jun 7, 2016

Contributor

This flag is disabled by default. I mean, you will see those extra weeks only when you have this flag enabled. I won't be weird because is what people will expect. The problem is that those "graphic glitches" can only be handled by this component. An external wrapper can not do any trick to avoid blank spaces.

Was #133 trying to solve the same problem?

Contributor

fcsonline commented Jun 7, 2016

This flag is disabled by default. I mean, you will see those extra weeks only when you have this flag enabled. I won't be weird because is what people will expect. The problem is that those "graphic glitches" can only be handled by this component. An external wrapper can not do any trick to avoid blank spaces.

Was #133 trying to solve the same problem?

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jun 7, 2016

Owner

Thanks for replying! Yes, in the OS X date picker I see this behavior implemented as well – so I changed my mind :) I never payed attention to this.

I'd call this prop fixedWeeks instead, or something else shorter than enableFixedAmountOfWeeks. (yes there's a enableOutsideDays, but that was a bad choice, may become deprecated soon). What do you think?

Owner

gpbl commented Jun 7, 2016

Thanks for replying! Yes, in the OS X date picker I see this behavior implemented as well – so I changed my mind :) I never payed attention to this.

I'd call this prop fixedWeeks instead, or something else shorter than enableFixedAmountOfWeeks. (yes there's a enableOutsideDays, but that was a bad choice, may become deprecated soon). What do you think?

@fcsonline

This comment has been minimized.

Show comment
Hide comment
@fcsonline

fcsonline Jun 7, 2016

Contributor

Great! I'm going to change the flag name and fix those eslint errors. 👏

Contributor

fcsonline commented Jun 7, 2016

Great! I'm going to change the flag name and fix those eslint errors. 👏

Added fixedWeeks flag
With this new flag, the calendar will show always the same number of
weeks. Exactly 6, because is the maximum number of week that a month can
have.

@fcsonline fcsonline changed the title from Added enableFixedAmountOfWeeks flag to Added fixedWeeks flag Jun 8, 2016

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 8, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling e3a4471 on fcsonline:feature/6-weeks into ec6795e on gpbl:master.

coveralls commented Jun 8, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling e3a4471 on fcsonline:feature/6-weeks into ec6795e on gpbl:master.

@gpbl gpbl merged commit fc72701 into gpbl:master Jun 8, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jun 8, 2016

Owner

Awesome thanks

Owner

gpbl commented Jun 8, 2016

Awesome thanks

@fcsonline

This comment has been minimized.

Show comment
Hide comment
@fcsonline

fcsonline Jun 8, 2016

Contributor

👏 Thanks!

Contributor

fcsonline commented Jun 8, 2016

👏 Thanks!

@fcsonline

This comment has been minimized.

Show comment
Hide comment
@fcsonline

fcsonline Jun 8, 2016

Contributor

Could you release it, please?

Contributor

fcsonline commented Jun 8, 2016

Could you release it, please?

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jun 8, 2016

Owner

I want first to update the docs and the examples :) While trying this prop, I wonder if fixedWeeks should automatically set enableOutsideDays to true. It doesn't make much sense to enable 6 weeks without displaying the outside days, does it?

Owner

gpbl commented Jun 8, 2016

I want first to update the docs and the examples :) While trying this prop, I wonder if fixedWeeks should automatically set enableOutsideDays to true. It doesn't make much sense to enable 6 weeks without displaying the outside days, does it?

@fcsonline

This comment has been minimized.

Show comment
Hide comment
@fcsonline

fcsonline Jun 9, 2016

Contributor

I was thinking about that during development. Maybe you want to avoid the graphic glitches but don't want to see outside days. Weird case indeed. I don't have a strong opinion about it.

Contributor

fcsonline commented Jun 9, 2016

I was thinking about that during development. Maybe you want to avoid the graphic glitches but don't want to see outside days. Weird case indeed. I don't have a strong opinion about it.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jun 9, 2016

Owner

@fcsonline So my position here is to enable them. Outside days can always be disabled by setting the prop to false.

Owner

gpbl commented Jun 9, 2016

@fcsonline So my position here is to enable them. Outside days can always be disabled by setting the prop to false.

@fcsonline

This comment has been minimized.

Show comment
Hide comment
@fcsonline

fcsonline Jun 9, 2016

Contributor

So you mean to enable it if is undefined, right? 👌

Contributor

fcsonline commented Jun 9, 2016

So you mean to enable it if is undefined, right? 👌

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jun 9, 2016

Owner

Uhm nope the default value for the prop is false :( We would mess the code just for an edge case that doesn't make sense anyway :) Release coming soon!

Owner

gpbl commented Jun 9, 2016

Uhm nope the default value for the prop is false :( We would mess the code just for an edge case that doesn't make sense anyway :) Release coming soon!

@fcsonline

This comment has been minimized.

Show comment
Hide comment
@fcsonline

fcsonline Jun 9, 2016

Contributor

Cool!

Contributor

fcsonline commented Jun 9, 2016

Cool!

gpbl added a commit that referenced this pull request Jun 9, 2016

Merge pull request #178 from gpbl/fixedweeks
Add docs, fix behavior of outside days for fixedWeeks (#176)
@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jun 9, 2016

Owner

Published as v2.2.0! example

Owner

gpbl commented Jun 9, 2016

Published as v2.2.0! example

@fcsonline

This comment has been minimized.

Show comment
Hide comment
@fcsonline

fcsonline Jun 9, 2016

Contributor

Thanks!

Contributor

fcsonline commented Jun 9, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment