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

[GHG-654] Added mentions on sidebar buttons / rhs bar #775

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

cyrusjc
Copy link
Contributor

@cyrusjc cyrusjc commented May 3, 2024

Closes #654

Summary:
Added mention icon and associated information in the RHS bar for the mentions in PRs as suggested in the issue.

  1. Server side changes include additional mentions graphql search query.
  2. Webapp changes includes the mentions on the github sidebar buttons + sidebar right

Changes looks like:
image
image
image

@cyrusjc cyrusjc requested review from hanzei and mickmister as code owners May 3, 2024 00:47
@mattermost-build
Copy link
Contributor

Hello @cyrusjc,

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.

@cyrusjc cyrusjc changed the title [GHG 654] Added mentions on sidebar buttons / rhs bar [GHG-654] Added mentions on sidebar buttons / rhs bar May 3, 2024
style={button}
>
<i className='fa fa-comment-o'/>
{' ' + mentions.length}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm leaning towards not showing the number of mentions.

There are users who are keen on keeping their inboxes empty. Given that they have no control over this number and can't reduce it, I wonder if we should omit the number. The button would still allow the user to check in on their conversation but would no longer nag them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I think it makes sense to remove the number. Would we then want to change the ordering of the mentions icon? I was thinking since it would be just an icon we can move it next to the refresh button

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would look something like this:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@hanzei Is it possible to "remove" mentions? Maybe unsubscribing to the issue/PR does this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cyrusjc Do you know?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would look something like this:

image

I like the suggestion 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't sorry.

@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester 3: Security Review Review requested from Security Team and removed 3: Security Review Review requested from Security Team labels May 6, 2024
@mickmister mickmister requested a review from ayusht2810 May 7, 2024 19:57
server/plugin/api.go Outdated Show resolved Hide resolved
server/plugin/graphql/lhs_request.go Outdated Show resolved Hide resolved
webapp/src/selectors.js Outdated Show resolved Hide resolved
cyrusjc and others added 3 commits May 8, 2024 12:29
Co-authored-by: Ayush Thakur <[email protected]>
consistency renaming

Co-authored-by: Ayush Thakur <[email protected]>
consistency renaming

Co-authored-by: Ayush Thakur <[email protected]>
@mickmister
Copy link
Contributor

mickmister commented May 14, 2024

@cyrusjc There are some merge conflicts to resolve due to a recent typescript PR merge. Can you please take a look at them? Thanks for your work on this!! 😄

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @cyrusjc 👍 Just one comment for discussion

webapp/src/reducers/index.ts Outdated Show resolved Hide resolved
webapp/src/selectors.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

LGTM

@mattermost-build
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester Contributor Lifecycle/1:stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mentioned Pull Panel Available
5 participants