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

feat(mentions,tags): tag mentions #3769

Merged
merged 22 commits into from
Apr 19, 2023
Merged

feat(mentions,tags): tag mentions #3769

merged 22 commits into from
Apr 19, 2023

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Mar 18, 2023

Also Closes #3725
Also Closes #3693

Changes proposed in this pull request:

  • Refactors JS autocomplete code to allow easily adding different mention formats and different mentionable models. Notably, we now have the interfaces MentionableModel and MentionFormat.
  • Adds tags search on the tags API list endpoint.
  • Adds tag mentions tests.
  • Adds a tag mention feature using the format: #slug since the slug is unique.
  • Adds tag mention stying.
  • Fixes unauthorized mention of hidden groups.
  • Makes sure the mentions extension works with and without the tags extension installed/enabled.

Reviewers should focus on:

  • The actual post_mentions_tag table migration resides i the tags extension whereas the rest of the tag mention logic resides in the mentions extension. This is done for the following reasons:
    • It shouldn't matter because by 2.0 we will have a polymorphic backend implementation of mentionable models, in which case this table will be dropped anyway.
    • Even if the mentions extension isn't used, the existence of the table does not add inconvenience.
    • There is a very good chance we will use this table to fix ([Tags] Event posts reference [Deleted] tag issue-archive#44) as part of tags anyway, in which case it would make sense to have it in tags.
  • The mentions extension introduces an extension API to easily add new mention formats and new mentionable models, for example, if I was building a discussion mention extension that used a format of !id:
return [
  new Extend.Mentionables()
    .format(ExclamationFormat)
    .mentionable('!', DiscussionMention),
];

Screenshot
Screencast from 03-13-2023 10:45:59 AM.webm

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

SychO9 and others added 20 commits March 18, 2023 18:28
Signed-off-by: Sami Mazouz <[email protected]>
Signed-off-by: Sami Mazouz <[email protected]>
Signed-off-by: Sami Mazouz <[email protected]>
Signed-off-by: Sami Mazouz <[email protected]>
Signed-off-by: Sami Mazouz <[email protected]>
Signed-off-by: Sami Mazouz <[email protected]>
@SychO9 SychO9 added this to the 1.8 milestone Mar 18, 2023
@SychO9 SychO9 self-assigned this Mar 18, 2023
@SychO9 SychO9 requested a review from a team as a code owner March 18, 2023 22:44
@SychO9 SychO9 mentioned this pull request Mar 18, 2023
10 tasks
Copy link
Member

@luceos luceos left a comment

Choose a reason for hiding this comment

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

This works as intended, but is there a way to use routing instead of a href link?

@davwheat
Copy link
Member

davwheat commented Apr 4, 2023

Without that, the linking wouldn't be accessible to screenreaders, nor indexable by search engines, though?

@SychO9
Copy link
Member Author

SychO9 commented Apr 10, 2023

Without that, the linking wouldn't be accessible to screenreaders, nor indexable by search engines, though?

It shouldn't be the case, the code already uses a click event use the mithril routing for user and post mentions.

@luceos
Copy link
Member

luceos commented Apr 17, 2023

I'd be okay for this to be merged for v1.8, we can properly test this in QA. In fact we should update our PR template to include a checklist item for "Provide steps to properly test this functionality still works during QA" (on it)

# Conflicts:
#	extensions/mentions/extend.php
#	extensions/mentions/js/src/@types/shims.d.ts
#	extensions/mentions/src/FilterVisiblePosts.php
#	extensions/mentions/tests/integration/api/ListPostsTest.php
@imorland imorland merged commit 5e28113 into main Apr 19, 2023
@imorland imorland deleted the sm/tag-mentions branch April 19, 2023 11:58
@github-actions github-actions bot mentioned this pull request May 17, 2023
@SychO9 SychO9 added javascript Pull requests that update Javascript code and removed prio/high labels May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code type/feature
Projects
None yet
5 participants