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

Fix FileTileProvider to work on the web #1170

Merged
merged 3 commits into from
Feb 27, 2022
Merged

Fix FileTileProvider to work on the web #1170

merged 3 commits into from
Feb 27, 2022

Conversation

mu-dawood
Copy link
Contributor

This pr create two files file_tile_provider_io.dart and file_tile_provider_web.dart
and export the right one

@JaffaKetchup
Copy link
Member

Thanks for this - I saw your note on the other PR. Just running checks now, should be able to review tomorrow or soon.

@JaffaKetchup JaffaKetchup self-assigned this Feb 21, 2022
@JaffaKetchup JaffaKetchup self-requested a review February 21, 2022 20:20
Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

LGTM. Haven't fully tested, but it doesn't look like it should cause any problems. If it's not too much of a hassle, can you remove the 'Podfile', as it's unrelated to this commit (I think)? Otherwise I'm happy to merge.

Copy link
Collaborator

@ibrierley ibrierley left a comment

Choose a reason for hiding this comment

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

I'm a bit undecided on this one, as not enough experience on the flutter web side to know what's best. It doesn't feel like the Podfile is needed as previously mentioned, so I think that could go.

Regarding the other bits, something "feels" odd about having a FileTileProvider that uses a NetworkImage...but I can understand why the web makes this a bit messy. Wouldn't one simply not use it, or does it create an error when building for the web (again, I'm not so familiar with that)?

@JaffaKetchup JaffaKetchup self-requested a review February 26, 2022 16:18
Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

As @ibrierley also said, please remove the 'Podfile' as it is not directly related to this PR.
If you can also explain why NetworkImage is used, that would be good: I'm assuming that is just how the Flutter framework works, but I might be wrong.

@mu-dawood
Copy link
Contributor Author

The browser can not access file system directly it can accept blobs

so in flutter web you have to load the image using NetworkImage instead of FileImage

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

LGTM, and that explanation makes sense. @ibrierley?

@ibrierley
Copy link
Collaborator

Yeah, I think I'm happy with that, and I think it's all straightforward enough to amend should a better strategy evolve with the web stuff. Will give a retest and merge if all still looks ok.

@ibrierley
Copy link
Collaborator

All good on Android and Web for me.

@ibrierley ibrierley merged commit e408ce7 into fleaflet:master Feb 27, 2022
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.

3 participants