search

mattermost / mattermost-plugin-github

GitHub plugin for Mattermost
Apache License 2.0
156 stars 146 forks source link

[GH-730] Add link preview for PR and issue. #779

Open Sn-Kinos opened 2 months ago

Sn-Kinos commented 2 months ago

Summary

Screenshots

Denim Onyx
image image

Ticket Link

mattermost-build commented 2 months ago

Hello @Sn-Kinos,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

mickmister commented 2 months ago

@matthewbirtch I'm curious about your thoughts on this feature. It's essentially a clone of the link tooltip component in the form of a link embed preview. Do you think this duplication could be noisy or cause confusion? Note that the preview will only show for users that have their GitHub account connected.

matthewbirtch commented 2 months ago

@matthewbirtch I'm curious about your thoughts on this feature. It's essentially a clone of the link tooltip component in the form of a link embed preview. Do you think this duplication could be noisy or cause confusion? Note that the preview will only show for users that have their GitHub account connected.

@mickmister I definitely see this as redundant with the hover popover that's been added. In my mind we would need to choose one or the other. Or make it configurable which kind of preview to use in the workspace.

Sn-Kinos commented 2 months ago

@matthewbirtch I'm curious about your thoughts on this feature. It's essentially a clone of the link tooltip component in the form of a link embed preview. Do you think this duplication could be noisy or cause confusion? Note that the preview will only show for users that have their GitHub account connected.

@mickmister I definitely see this as redundant with the hover popover that's been added. In my mind we would need to choose one or the other. Or make it configurable which kind of preview to use in the workspace.

Without this feature, We can watch the preview like below on private repository. I thought it is uncomfortable, so I added this feature. image

mickmister commented 2 months ago

@Sn-Kinos @matthewbirtch We can make this feature configurable per-user so they can choose for themselves. We would want to make it obvious that the user can configure these two features separately.

Also note that this feature will passively be making requests to GitHub potentially often compared to the link tooltip feature, which could have negative effects on the server in terms of performance.

On our community server, we use the autolink plugin which with our configuration of the plugin makes it so GitHub PR links are shortened and hyperlinked, which makes it so the webapp does not use the "embed" feature that this PR relies on in that case. So if I were to opt for the embed feature and not the tooltip, I wouldn't have the chance to use this feature that often on our server. Just something to point out since this could be the case for other servers as well.

matthewbirtch commented 2 months ago

@Sn-Kinos @matthewbirtch We can make this feature configurable per-user so they can choose for themselves. We would want to make it obvious that the user can configure these two features separately.

@mickmister Rather than adding a user setting and increasing that complexity, I would suggest we go with a system-level setting in the admin console. That way everyone on the server sees things the same way. That can be very beneficial to ensure everyone has the same context.

Sn-Kinos commented 2 months ago

I'm not sure I understand how that works. It shows the fallback because this component returns null in that case?

That's what I think. I don't know deeply about the fundamentals of registerPostWillRenderEmbedComponent so I have referenced link_tooltip.

Sn-Kinos commented 2 months ago

@Sn-Kinos @matthewbirtch We can make this feature configurable per-user so they can choose for themselves. We would want to make it obvious that the user can configure these two features separately.

@mickmister Rather than adding a user setting and increasing that complexity, I would suggest we go with a system-level setting in the admin console. That way everyone on the server sees things the same way. That can be very beneficial to ensure everyone has the same context.

I think It is good to only work when EnablePrivateRepo is enabled. If It's disabled, this feature is redundant because the main issue of this PR is showing link preview of private repository.

Sn-Kinos commented 2 months ago

Great job on this @Sn-Kinos 👍

From our discussion, I have one remaining ask to add a plugin setting to toggle between this feature and the link tooltip. This will go in the plugin.json file in the root of the repository

  • Name: EnableLinkPreviewEmbeds
  • Description: When enabled, whenever a link to an issue or pull request is shared in a channel, the embed content will be loaded for the specific user viewing it. This allows users to view previews from private repositories using their own connected account. Note that enabling this also disables the "link tooltip" feature in the plugin.

Please let me know your thoughts on this @Sn-Kinos

@mickmister @matthewbirtch I don't think it's very essential to add setting. This link_embed_preview is necessary for private repository only. If the link is public repo, It shows embed preview in a normal way.

image

I think we can handle it with the 'EnablePrivateRepo' setting because the reason for developing link_embed_preview is that the information of private repository is not shown in the existing embedded preview.

mickmister commented 2 months ago

@Sn-Kinos I discussed with @matthewbirtch offline, and we're thinking there should be a system console setting that allows the server to be configured for either the link tooltip or link embed. The rationale is that there is more functionality here than simply filling in the gaps for the missing open graph information for private repos, because here there's a rich display of information about the issue/PR with this feature, rather than the simple open graph preview that already shows for public repositories.

To give the system admin a choice to enable one or the other, we can use a radio setting like this one being used in the Jira plugin: https://github.com/mattermost/mattermost-plugin-jira/blob/4b296a0c23d04f0575b4c86bc4c9f042137f52cc/plugin.json#L46

Please let me know your thoughts on this @Sn-Kinos

Sn-Kinos commented 2 months ago

@Sn-Kinos I discussed with @matthewbirtch offline, and we're thinking there should be a system console setting that allows the server to be configured for either the link tooltip or link embed. The rationale is that there is more functionality here than simply filling in the gaps for the missing open graph information for private repos, because here there's a rich display of information about the issue/PR with this feature, rather than the simple open graph preview that already shows for public repositories.

To give the system admin a choice to enable one or the other, we can use a radio setting like this one being used in the Jira plugin: https://github.com/mattermost/mattermost-plugin-jira/blob/4b296a0c23d04f0575b4c86bc4c9f042137f52cc/plugin.json#L46

Please let me know your thoughts on this @Sn-Kinos

I concerned Also note that this feature will passively be making requests to GitHub potentially often compared to the link tooltip feature, which could have negative effects on the server in terms of performance. on the comment. If It doesn't matter, It will be ok to add the settings.

mickmister commented 1 month ago

I concerned Also note that this feature will passively be making requests to GitHub potentially often compared to the link tooltip feature, which could have negative effects on the server in terms of performance. on the comment. If It doesn't matter, It will be ok to add the settings.

@Sn-Kinos The main reason we want to add the settings is the UX flow of toggling showing the issue/PR through tooltip or link embed preview. I'm not sure I understand the point you're making in your last comment here

Sn-Kinos commented 1 month ago

@Sn-Kinos The main reason we want to add the settings is the UX flow of toggling showing the issue/PR through tooltip or link embed preview. I'm not sure I understand the point you're making in your last comment here

I worried 'negative effects on the server in terms of performance' on the comment. If It doesn't matter, I'll be ok :D

mickmister commented 1 month ago

@Sn-Kinos I think the piece I'm not understanding is "If It doesn't matter". Are you saying "if the performance issues don't matter"? Or if something else doesn't matter? Sorry about this

The only blocker here is for the UX piece. I think the performance issue is probably not a real issue here

Sn-Kinos commented 1 month ago

@Sn-Kinos I think the piece I'm not understanding is "If It doesn't matter". Are you saying "if the performance issues don't matter"? Or if something else doesn't matter? Sorry about this

The only blocker here is for the UX piece. I think the performance issue is probably not a real issue here

Ahhhh You're right. I concerned about the performance issue. Now, I agree with you as you mentioned.

Sorry about the miscommunication problem. the translator that I used didn't work properly I presume.

mattermost-build commented 1 month ago

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

Sn-Kinos commented 1 month ago

I wonder if there's anything else I need to do 🏃‍♂️

mickmister commented 1 month ago

@Sn-Kinos Sorry for the confusing conversation here. There is one remaining requirement here to implement a plugin setting to toggle this and the link tooltip feature, as discussed here https://github.com/mattermost/mattermost-plugin-github/pull/779#issuecomment-2129814712. Does this make sense?

Sn-Kinos commented 1 month ago

@mickmister I understand. What are the options I need to make?

mickmister commented 1 month ago

@Sn-Kinos We can have these two boolean plugin settings:

mattermost-build commented 1 week ago

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!