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

Remove nullable getter for ResponsiveValue #123

Conversation

StrDragos
Copy link

@StrDragos StrDragos commented Jan 24, 2023

Remove the null possibility of a ReponsiveValue on the value getter. It is not needed since ResponsiveValue also receives a default value.

Will also address: ResponsiveValue.value should non-nullable #119

@rayliverified
Copy link
Contributor

Thank you for the PR

@rayliverified
Copy link
Contributor

#119

@rayliverified
Copy link
Contributor

Really interesting API decisions with ResponsiveValue.

The choice was between making defaultValue required which would then allow value to inherit the nullability of the generic type. Or, keep defaultValue optional which necessitates value being optional as well, even if a nonnullable type is specified.

The latter was chosen for 2 reasons.

  1. defaultValue is a convenience field and should not be required. If there are a lot of conditions, it's better for readability to have the defaultValue at the front rather than behind ??.
  2. The Flutter convention for "default" or optional values is ??. ResponsiveValue().value ?? defaultValue is more in line with Flutter conventions.

The drawback with this approach is an explicit nonnull cast ResponsiveValue().value! is required. This isn't completely a drawback as it forces the developer to think about nullability and default values.

@tjarvstrand
Copy link

tjarvstrand commented Jun 15, 2023

The Flutter convention for "default" or optional values is ??. ResponsiveValue().value ?? defaultValue is more in line with Flutter conventions.

The problem with this approach is that if you pass the ResponsiveValue down the tree, the default value has to be known in every place that it is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants