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

Don't show excerpt in PostPreview component if there are no plain content #2964

Merged
merged 1 commit into from
Aug 13, 2021
Merged

Don't show excerpt in PostPreview component if there are no plain content #2964

merged 1 commit into from
Aug 13, 2021

Conversation

the-turk
Copy link
Contributor

Fixes #2942

Reviewers should focus on:
span.PostPreview-excerpt is still necessary for proper view.

Quoting from @SychO9:

Besides fixing the JS error, it would be a good idea to improve the preview for an event post mention, even though people aren't supposed to reply to event posts, they still can by manually writing the post mention, as long as they have the post's ID.

Since we don't store any information about event posts in db, I think this kind of enhancement will need some breaking changes.

Confirmed

  • Frontend changes: tested on a local Flarum installation.

Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

This is enough, I don't think there is any value in it containing info about the event post, since such post mentions shouldn't happen anyway.

we don't store any information about event posts in db

We do btw, the posts themselves have info on the event, thus event posts.

@the-turk
Copy link
Contributor Author

We do btw, the posts themselves have info on the event, thus event posts.

I mean, yeah, but we don't have the information on how to turn that into readable text like " x locked the discussion". All info we have is it's an event post which has 'locked' type and nothing more.

@SychO9 SychO9 requested a review from davwheat August 10, 2021 21:00
Copy link
Member

@davwheat davwheat left a comment

Choose a reason for hiding this comment

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

Coincidence! I was just taking a look at the issue for this PR :P

Looks good to me and appears to work locally 👍

@SychO9 SychO9 merged commit b32496d into flarum:master Aug 13, 2021
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.

Replying / Linking Event Post leads to JS Error
3 participants