club-1 / flarum-ext-cross-references

Add cross reference links when a discussion is mentioned from another one.
GNU Affero General Public License v3.0
6 stars 1 forks source link

Keep links of unknown discussions in comments at render time #36

Closed n-peugnet closed 1 year ago

n-peugnet commented 1 year ago

Fixes #35 (if we extract the DiscussionReferencedPost part in a new issue). I am still not sure if I want to add a link or not for short references in this case. @rob006 I am curious of your opinion about this.

Here is how it looks for now:

2023-02-19-170305_540x195_scrot

I think this also closes #31, as most of it will be implemented then. The last prominent issue remaining for you @rob006 would be to be able to disable the "Retrofit of old messages in the frontend", right? I will create one more issue for this. And I think this can be an option.


@rob006 I took the liberty to add you as co-author of this commit since this is mostly what you did in #31

codecov-commenter commented 1 year ago

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage :thumbsup:

Coverage data is based on head (fb1aa7c) compared to base (152437d). Patch has no changes to coverable lines.

:exclamation: Current head fb1aa7c differs from pull request most recent head a00e935. Consider uploading reports for the commit a00e935 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #36 +/- ## =========================================== Coverage 100.00% 100.00% Complexity 30 30 =========================================== Files 5 5 Lines 140 139 -1 =========================================== - Hits 140 139 -1 ``` | [Impacted Files](https://codecov.io/gh/club-1/flarum-ext-cross-references/pull/36?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=club-1) | Coverage Δ | | |---|---|---| | [src/Formatter/CrossReferencesConfigurator.php](https://codecov.io/gh/club-1/flarum-ext-cross-references/pull/36?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=club-1#diff-c3JjL0Zvcm1hdHRlci9Dcm9zc1JlZmVyZW5jZXNDb25maWd1cmF0b3IucGhw) | `100.00% <ø> (ø)` | | | [src/Formatter/CrossReferencesRenderer.php](https://codecov.io/gh/club-1/flarum-ext-cross-references/pull/36?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=club-1#diff-c3JjL0Zvcm1hdHRlci9Dcm9zc1JlZmVyZW5jZXNSZW5kZXJlci5waHA=) | `100.00% <ø> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=club-1). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=club-1)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

rob006 commented 1 year ago

I am still not sure if I want to add a link or not for short references in this case. @rob006 I am curious of your opinion about this.

I would keep link in form of [#11](https://example.com/d/11) as this was original intention and does not hide/disclose any additional information.

I think this also closes https://github.com/club-1/flarum-ext-cross-references/pull/31, as most of it will be implemented then.

Note that #31 contains one minor fix that prevents attaching multiple onclick events to links: https://github.com/club-1/flarum-ext-cross-references/pull/31/commits/2d0642bdeae14ca6294f3367c71f9b35d8df8fd7. It's not very elegant solution, but without it "back" button in browser did not worked correctly. Besides that I'm fine with closing my PR.

n-peugnet commented 1 year ago

I am still not sure if I want to add a link or not for short references in this case. @rob006 I am curious of your opinion about this.

I would keep link in form of [#11](https://example.com/d/11) as this was original intention and does not hide/disclose any additional information.

Seems reasonable. Did it in my last commit.

Note that #31 contains one minor fix that prevents attaching multiple onclick events to links: 2d0642b. It's not very elegant solution, but without it "back" button in browser did not worked correctly.

I can't seem to be able to replicate this issue. I didn't find instances of duplicated events and I didn't notice issues with the "back" button of the browser. Could please you open an issue with a reproducer?