-
Notifications
You must be signed in to change notification settings - Fork 146
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
Added setting to hide/show left sidebar github buttons #396
Conversation
Hello @jatinjtg, 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @jatinjtg ! Some small text description changes, because many people don't know what the "LHS" of Left Hand Sidebar. I added text to explain what the feature is doing.
Co-authored-by: Aaron Rothschild <aaron.rothschild@gmail.com>
Co-authored-by: Aaron Rothschild <aaron.rothschild@gmail.com>
Co-authored-by: Aaron Rothschild <aaron.rothschild@gmail.com>
Co-authored-by: Aaron Rothschild <aaron.rothschild@gmail.com>
Co-authored-by: Aaron Rothschild <aaron.rothschild@gmail.com>
Co-authored-by: Aaron Rothschild <aaron.rothschild@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. Just 2 comments.
Codecov Report
@@ Coverage Diff @@
## master #396 +/- ##
=======================================
Coverage 19.34% 19.35%
=======================================
Files 11 11
Lines 2766 2770 +4
=======================================
+ Hits 535 536 +1
- Misses 2193 2196 +3
Partials 38 38
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. A few comments on ordering
server/plugin/api.go
Outdated
@@ -130,6 +134,7 @@ func (p *Plugin) initializeAPI() { | |||
|
|||
apiRouter.HandleFunc("/config", checkPluginRequest(p.getConfig)).Methods(http.MethodGet) | |||
apiRouter.HandleFunc("/token", checkPluginRequest(p.getToken)).Methods(http.MethodGet) | |||
apiRouter.HandleFunc("/settings", p.getSettings).Methods(http.MethodGet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please move this after apiRouter.HandleFunc("/connected", p.getConnected).Methods(http.MethodGet)
to keep the list sorted by middle ware type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like that change wasn't pushed. Could you please double check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @hanzei for late reply. I have fixed the ordering.
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
Thanks for the PR, @jatinjtg! Do you need any assistance regarding the PR feedback? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, LGTM 👍
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
@DHaussermann, gentle ping for review~ |
/update-branch |
We don't have permissions to update this PR, please contact the submitter to apply the update. |
@jfrerich - Not sure if there are many changes since the initial submission. Are you able to merge master in? |
I don't have the ability to merge master into the author's repo. I created a branch in the mattermost repo that includes merged master into this branch. You can test this if you'd like.. https://github.com/mattermost/mattermost-plugin-github/commits/left_sidebar These are the steps I took. to create the testable branch of this PR, with the latest master merged From inside
|
@DHaussermann You can just test this PR as it is. There is to eminent need to sync it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and working as expected.
- Options show and hide as expected
- Works for one team with horizontal bar on LHS and for team sidebar
- Work for octocat icon when user is not connected
- New flag to show options default to true for seamless upgrade
Thanks @jamiehurewitz for this PR!
Not blocking for this change but @aaronrothschild, as described in this task, the flag hides the LHS and team sidebar items. We would need a separate PR if the goal is to hide all UI elements.
/update-branch |
We don't have permissions to update this PR, please contact the submitter to apply the update. |
Thanks @jatinjtg for the contribution! |
@chetanyakan Can you verify for me that you just wanted the LHS to be hidden by a system level setting? In Jira, we hide all webapp components (including the post-action message to log a Jira ticket in the drop down menu). Is hiding the LHS notifications sufficient for your request or do we need another ticket to cover "hiding the post-action menu item to create New/Attach GitHub issues" |
@aaronrothschild Thanks for confirming. Another ticket for further changes is not required at the moment. The changes from this PR are enough to satisfy my requirements for the time being, since having multiple plugins that register an LHS component is not as obtrusive anymore. |
Mattermost github plugin issue - #384