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

replace newlines in desktop notifs with spaces instead of just removing them #5361

Merged
merged 1 commit into from Oct 14, 2017

Conversation

MightyPork
Copy link
Contributor

fix desktop notifications removing spaces without any replacement, causing words to be joined
this bug was there for several months now, i finally got enough of it lol
screenshot_20171012_202118

added all possible newline types, seems to have no negative effects and better to have it more general just in case imo

@Gargron
Copy link
Member

Gargron commented Oct 12, 2017

I'm not sure if desktop notifications honour \n newlines or not, but if they do, wouldn't it be better to replace br with those

@MightyPork
Copy link
Contributor Author

MightyPork commented Oct 12, 2017

I think it supports \n, but it doesn't seem to be very reliable - the w3 example page uses " \n" (for compatibility?)

this has advantages:

  • multiline toots won't show ridiculously huge notifications
  • it's easy (textContent seems to discard newlines, so some workaround would be needed)
  • it collapses multiple newlines / spaces to a single space

@sorin-davidoi
Copy link
Contributor

This also affects push notifications (#4860) - the same fix may be required, but in the backend (I think this line is responsible). @MightyPork do you think you can address this as well?

@MightyPork
Copy link
Contributor Author

@sorin-davidoi I don't know ruby, so it'd be something horrible glued together from stackoverflow, better if we merge this first and you can add that later i think

@sorin-davidoi
Copy link
Contributor

I don't know ruby, so it'd be something horrible glued together from stackoverflow

That would not be something bad, as that is how the push notifications were implemented 😄

@Gargron Gargron merged commit ae716a1 into mastodon:master Oct 14, 2017
cobodo pushed a commit to cobodo/mastodon that referenced this pull request Oct 20, 2017
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.

None yet

3 participants