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

Started v2.0 migration #1986

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Conversation

theswerd
Copy link
Contributor

In progress

Checklist:

Copy link
Collaborator

@brunobowden brunobowden left a comment

Choose a reason for hiding this comment

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

@theswerd - this looks good to me. What else does it need?

@@ -69,7 +69,7 @@ class __HomeStatsFaderState extends State<_HomeStatsFader>
with SingleTickerProviderStateMixin {
bool fadeState = true;

AnimationController _animationController;
late AnimationController _animationController;
Copy link
Collaborator

Choose a reason for hiding this comment

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

late is a good syntax

notification_permissions: ^0.5.0
# observer_provider: 0.0.1+2
package_info: "2.0.0"
# page_view_indicator: ^2.0.4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make this a TODO?

@theswerd
Copy link
Contributor Author

just looked over the cache manager - reimplementation should be relatively easy. they provided instructions for moving away from base cache manager.

@brunobowden
Copy link
Collaborator

brunobowden commented Apr 29, 2021 via email

@theswerd
Copy link
Contributor Author

Cache manager working - only need to re-implement charts

Copy link
Collaborator

@brunobowden brunobowden left a comment

Choose a reason for hiding this comment

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

Changes to cache manager look good

All we need is fl_chart now...

@@ -45,7 +45,7 @@ class _CountryListPageState extends State<CountryListPage> {
decoration: InputDecoration(
border: InputBorder.none, hintText: 'Enter your country'),
onChanged: (String value) async {
List filtered = (widget.countries?.values ?? []).where((element) {
List filtered = (widget.countries.values).where((element) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is much cleaner. Good case where null safety enforces better discipline

@stale
Copy link

stale bot commented May 15, 2021

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the resolved:stale No recent activity on the issue or PR label May 15, 2021
@stale stale bot removed the resolved:stale No recent activity on the issue or PR label Jun 2, 2021
Copy link
Collaborator

@brunobowden brunobowden left a comment

Choose a reason for hiding this comment

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

@theswerd @bezfeng - great to get this working. I'll send another PR to update the build settings

Comment on lines +99 to +101
var country;
if (currentCountryCode != null)
country = countryList.countries[currentCountryCode];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking you could simplify this with Conditional Property Access... then I realize that doesn't work and you need to do it this way instead ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good call on simplifying this bit! After playing around some more, I think we can just do:

final country = countryList.countries[currentCountryCode];

(sorry, I could've sworn this was causing a runtime error but it's working fine for me now 😛)

@stale
Copy link

stale bot commented Jun 10, 2021

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the resolved:stale No recent activity on the issue or PR label Jun 10, 2021
@theswerd theswerd removed the resolved:stale No recent activity on the issue or PR label Jun 13, 2021
@stale
Copy link

stale bot commented Jun 20, 2021

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the resolved:stale No recent activity on the issue or PR label Jun 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolved:stale No recent activity on the issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants