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: disallow media description exceeding limits #1854

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

zaidhaan
Copy link
Contributor

@zaidhaan zaidhaan commented Mar 5, 2023

Mastodon enforces a 1500 character limit on media attachment descriptions. This is visible in their interface too:
image

Elk on the other hand has no such indicator:
image

As such, it enforces no limits on the attachment length, and so if you post something exceeding 1500 characters, you get this in your console:
image

This error is not visible to the user (due to it not being captured in failedMessages since the error occurs in setDescription (publish.ts) which has no access to that array)

In any case, the potential solutions to this issue would be as follows:

  • Let the user enter a large description, but display the error once they apply the description
  • Indicate to the user how many characters have been entered and how many are left, and prevent setting descriptions that exceed that value

I chose to go with the latter solution, since the first one was a bit difficult to implement given the current architecture of the code.

So with this PR, one would see this after entering too many characters:
image

Additionally, since this PR prevents setting long descriptions (i.e. disables the button), I figured I'd also include that inside the status submission ("Publish") button, since that effectively prevents the user from calling an unnecessary API request that will always fail, and will always throw an error message to the user. So long statuses won't be publishable:
image

(Perhaps not the most elegant solution -- disabled button does not look the greatest, and I had to make a simple character component just to avoid code duplication -- but it works)

Fixes #1018.

@stackblitz
Copy link

stackblitz bot commented Mar 5, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Mar 5, 2023

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit 279c662
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/6404a2e058aa2a000850afa8

@netlify
Copy link

netlify bot commented Mar 5, 2023

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit 279c662
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/6404a2e0d2d0ee0008953f9d
😎 Deploy Preview https://deploy-preview-1854--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@patak-dev
Copy link
Member

Awesome! We can later check the styling of the button, we are due a general review of all buttons in the app.
Let's merge it, this works great 👍🏼

@patak-dev patak-dev merged commit cb0b7b5 into elk-zone:main Mar 6, 2023
@zaidhaan zaidhaan deleted the fix-long-media-description branch March 6, 2023 16:21
DataDrivenMD pushed a commit to Distal-Labs/elk that referenced this pull request Mar 11, 2023
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.

alt text is silently dropped if it overruns the server limit
2 participants