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

feat: use FileSystemFileHandle to modify a file on the local filesystem #965

Merged
merged 11 commits into from
Jan 9, 2025

Conversation

josxha
Copy link
Contributor

@josxha josxha commented Jan 6, 2025

Launch Checklist

  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Add an entry to CHANGELOG.md under the ## main section.

Changes

  • This pull request makes use of the FileSystemFileHandle API to modify a local file. No need to download it - just click save.
  • I don't know how to cover this functionality by tests so I need directions in case test coverage is required.
  • The pull request adds @types/wicg-file-system-access as a new dev dependency which I am not really pleased about but can't think of a way around it.

Known Limitations

  • The used File API is only available in when accessed from https or on localhost.
  • There is no visual indicator that the file has been saved. Previously the browser showed it as a new download.

Issue

Screenshots

Menu

Open modal

Save modal

@josxha josxha marked this pull request as draft January 6, 2025 18:19
@josxha josxha force-pushed the feat/edit-local-file branch from 74ca4f1 to 0999651 Compare January 6, 2025 19:02
@josxha josxha marked this pull request as ready for review January 6, 2025 19:18
@josxha josxha changed the title feat: use FileSystemFileHandle to modify a file in the local file system feat: use FileSystemFileHandle to modify a file on the local file system Jan 6, 2025
@josxha josxha changed the title feat: use FileSystemFileHandle to modify a file on the local file system feat: use FileSystemFileHandle to modify a file on the local filesystem Jan 6, 2025
@HarelM
Copy link
Collaborator

HarelM commented Jan 6, 2025

Yes, I believe there is a need to cover this in tests although I struggled with Cypress and file open button.
Can you add a video with the new behavior?

@josxha
Copy link
Contributor Author

josxha commented Jan 6, 2025

Yes, I believe there is a need to cover this in tests although I struggled with Cypress and file open button.

The this pull request removes the html input element. I found a way to test with it but it would require to somehow mock the File System Access API and I can't find a solution for this.

Can you add a video with the new behavior?

maputnik-showcase1.mp4

@HarelM
Copy link
Collaborator

HarelM commented Jan 7, 2025

Overall this looks like a great improvement, I'm happy to see that file API finally made it to the browsers...

@HarelM
Copy link
Collaborator

HarelM commented Jan 7, 2025

Let me know if you managed to find a way to test this...

@josxha
Copy link
Contributor Author

josxha commented Jan 7, 2025

From my pr description:

I don't know how to cover this functionality by tests so I need directions in case test coverage is required.

I didn't break any tests with this pr, so the functionality hasn't been covered previously. I'd write some tests to cover it but will need some guidance.

@HarelM
Copy link
Collaborator

HarelM commented Jan 7, 2025

I can't find anything when using Google about Cypress and this new API.
If you find anything let me know, if not then I'll merge as is...

@josxha
Copy link
Contributor Author

josxha commented Jan 9, 2025

If you find anything let me know, if not then I'll merge as is...

I looked into it but couldn't find a way with Cypress tests, too. Maybe another test setup for unit tests would be beneficial?

I changed "Download HTML" to "Create HTML" to be more in line. From my point of view the pr is ready to get merged. (:

@HarelM HarelM merged commit d50ea76 into maplibre:main Jan 9, 2025
7 checks passed
@HarelM
Copy link
Collaborator

HarelM commented Jan 9, 2025

Thanks!
Should be published in a few minutes.

@josxha josxha deleted the feat/edit-local-file branch January 9, 2025 16:56
@josxha josxha restored the feat/edit-local-file branch January 9, 2025 18:36
@josxha josxha deleted the feat/edit-local-file branch January 9, 2025 18:37
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.

2 participants