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 #3263: Adding base64 file upload component #3431

Merged
merged 8 commits into from
Feb 20, 2025
Merged

Conversation

alexcottner
Copy link
Contributor

@alexcottner alexcottner commented Feb 12, 2025

Resolves #3263.
Adding a base64 encoding file upload option. See demo video below for how this prototype works.

This PR:

  1. Adds a new RJSF widget to allow files to be "uploaded" as bse64 strings
  2. Updates the Read-me with an example of this (should we update remote-settings docs too?)
  3. Adds unit tests for the new component
  4. Adds some validation to prevent too-large files from being uploaded (1MB cap)

See the demo video and screenshots below.

simplescreenrecorder-2025-02-12_12.13.19.mp4

Validation error for too large files:
image

@alexcottner
Copy link
Contributor Author

@leplatrem and @nathan-barrett - How does this feel?

@leplatrem
Copy link
Contributor

Nice!
micronit: I would maybe file in the widget name? base64file?

@alexcottner alexcottner changed the title WIP - Issue #3263 - Adding base64 file upload component Issue #3263 - Adding base64 file upload component Feb 14, 2025
@alexcottner alexcottner marked this pull request as ready for review February 14, 2025 17:45
const hugeTestTxt = `data:text/plain;base64,${"aGkK".repeat(1024 * 512)}`; // "hi" repeatedly

class fileReaderMock {
error: FileReader["error"] = null;
Copy link
Contributor Author

@alexcottner alexcottner Feb 14, 2025

Choose a reason for hiding this comment

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

This mock may be worth committing to memory. This was the most difficult thing in the PR to develop.

@alexcottner alexcottner requested a review from say-yawn February 14, 2025 18:02
@nathan-barrett
Copy link

Awesome getting this in, lgtm

Copy link
Contributor

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

Nice! Thanks Alex for working on this.

I would recommend adding a test that does an assertion on the string field value that we obtain (eg data:content/type;base64,)

Important

We should probably add info about this in the records vs attachment section of our RS docs https://remote-settings.readthedocs.io/en/latest/getting-started.html#records-vs-attachments

);
}

async function getBase64Str(evt, changeCallback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it's called onChange elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think I was just avoiding that as we were getting a prop from rjsf called onChange. But I can just rename that one to rjsfOnChange.

Copy link
Contributor

Choose a reason for hiding this comment

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

then forget it 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, already did the thing

@leplatrem leplatrem changed the title Issue #3263 - Adding base64 file upload component Fix #3263: Adding base64 file upload component Feb 20, 2025
@leplatrem
Copy link
Contributor

Renamed the PR title, because:

➜  kinto-admin git:(master) ✗ git log | grep -i 'fix' | wc -l
     296
➜  kinto-admin git:(master) ✗ git log | grep -i 'issue' | wc -l
      11

alexcottner and others added 2 commits February 20, 2025 08:29
Co-authored-by: Mathieu Leplatre <mathieu@mozilla.com>
@alexcottner alexcottner merged commit 4b305cb into master Feb 20, 2025
6 checks passed
@alexcottner alexcottner deleted the issue-3263 branch February 20, 2025 18:42
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.

Add a new widget to store arbitrary files as base64 using a file input
3 participants