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

Null safety improvements #272

Merged
merged 2 commits into from Apr 9, 2021
Merged

Conversation

trygvis
Copy link
Contributor

@trygvis trygvis commented Apr 7, 2021

Allow customDayBuilder and markedDateIconBuilder to return null. If it
returns null, use the default. This restores the original behavior from
v1. Fixes #260.

Allow customDayBuilder and markedDateIconBuilder to return null. If it
returns null, use the default. This restores the original behavior from
v1. Fixes dooboolab-community#270.
@trygvis
Copy link
Contributor Author

trygvis commented Apr 7, 2021

Sorry, this fixes #270, not #260.

@trygvis
Copy link
Contributor Author

trygvis commented Apr 7, 2021

Hey @IchordeDionysos and @hyochan. You seems to have been active with the null safety work, are you able to review and/or merge this change?

@@ -1136,24 +1142,32 @@ class _CalendarState<T extends EventInterface>
bool isNextMonthDay,
bool isThisMonthDay,
DateTime now) {
final customDayBuilder = widget.customDayBuilder;
final customDayBuilder = this.widget.customDayBuilder;
Copy link
Member

Choose a reason for hiding this comment

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

Why changing this to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because a variable named widget exists in the same scope (even if declared later). It won't compile unless explicitly referenced through this.

Copy link
Member

Choose a reason for hiding this comment

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

Writing this inside the stateful widget looks a bit awkward here. How about changing the above variable name to something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same to me, it is all within a small method. Should I change Widget widget to Widget tmp and remove the this prefix? I see tmp is used in other places.

Copy link
Member

Choose a reason for hiding this comment

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

How about eventWidget?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with dayContainer, it matches the method name and what it is trying to create.

Anything else I can fix with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hyochan poke! :)

@hyochan hyochan added the enhancement New feature or request label Apr 8, 2021
Copy link
Member

@hyochan hyochan left a comment

Choose a reason for hiding this comment

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

LGTM!

@hyochan hyochan merged commit 89f1f4d into dooboolab-community:master Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support null safety
2 participants