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

Restore Missing fileInputId Property and Add acceptedFileTypes Property #1089

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

chadrschroeder
Copy link

Fixes #1088 by restoring the missing fileInputId property.

Also adds an acceptedFileTypes property, similar to what was proposed in #789. That PR was opened over 3 years ago back when this code was still using CoffeeScript. I've implemented a similar change here in JavaScript, but I set this up as a simple string attribute value instead of an array. Let me know if you'd rather not have this option, but this is why I was trying to override the pickFiles function in the first place.

Copy link
Contributor

@afcapel afcapel left a comment

Choose a reason for hiding this comment

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

There are a couple of styling issues, but the changes make sense. If you can fix them I'll merge.

.DS_Store Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this file, please?

Copy link
Author

Choose a reason for hiding this comment

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

Removed. I've also updated .gitignore to help avoid this. I hope this is OK. I see it's included there in Kamal.

@@ -3,6 +3,8 @@ import { makeElement, removeNode } from "trix/core/helpers/dom"

const input = {
level2Enabled: true,
fileInputId: `trix-file-input-${Date.now().toString(16)}`,
acceptedFileTypes: '*',
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer double quotes for strings. The linter should flag this.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@chadrschroeder
Copy link
Author

Thanks for the quick review before. Let me know if there's anything else you want me to update here. It looks like the most recent tests failed but in a way that is unrelated to my changes.

@ghiculescu
Copy link

Is this okay to merge @afcapel ?

@afcapel
Copy link
Contributor

afcapel commented Jul 25, 2024

@ghiculescu I afraid I don’t maintain Trix anymore, so that ‘s up to the new maintainers.

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.

Clicking to Attach a File Deletes First Element on Page with ID of "undefined"
3 participants