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

[issue-234] Hide Toolbar #288

Closed
wants to merge 4 commits into from

Conversation

sibasankarnayak
Copy link
Contributor

@sibasankarnayak sibasankarnayak commented Apr 7, 2022

add logic to hide/show left gitlab sidebar actions

ticket here
Fixes #234

@sibasankarnayak sibasankarnayak requested a review from iomodo as a code owner April 7, 2022 16:25
@mattermod
Copy link
Contributor

Hello @sibasankarnayak,

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.

@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

Merging #288 (8f76fab) into master (aec8208) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
- Coverage   28.59%   28.53%   -0.06%     
==========================================
  Files          20       20              
  Lines        2595     2600       +5     
==========================================
  Hits          742      742              
- Misses       1769     1774       +5     
  Partials       84       84              
Impacted Files Coverage Δ
server/api.go 0.00% <0.00%> (ø)
server/configuration.go 40.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aec8208...8f76fab. Read the comment docs.

@mattermod
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!

@iomodo iomodo requested review from hanzei and removed request for iomodo April 27, 2022 07:17
@hanzei hanzei requested a review from mickmister April 27, 2022 17:59
@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester and removed Lifecycle/1:stale labels Apr 27, 2022
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.

Great work @sibasankarnayak 👍 Just a few requests

plugin.json Outdated Show resolved Hide resolved
server/api.go Outdated
@@ -70,6 +74,8 @@ func (p *Plugin) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Req
p.completeConnectUserToGitlab(w, r)
case "/api/v1/connected":
p.getConnected(w, r)
case "/api/v1/getsettings":
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
case "/api/v1/getsettings":
case "/api/v1/settings":

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickmister already a route is present with same

case "/api/v1/settings":

Copy link
Contributor

Choose a reason for hiding this comment

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

How about client_settings instead?

webapp/src/actions/index.js Outdated Show resolved Hide resolved
@@ -34,6 +34,10 @@ export default class Client {
return this.doPost(`${this.url}/user`, {user_id: userID});
};

getSettings = async () => {
return this.doGet(`${this.url}/getsettings`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return this.doGet(`${this.url}/getsettings`);
return this.doGet(`${this.url}/settings`);

Comment on lines +33 to +36
if (settings && settings.left_sidebar_enabled) {
registry.registerLeftSidebarHeaderComponent(SidebarHeader);
registry.registerBottomTeamSidebarComponent(TeamSidebar);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a websocket message that tells the frontend when this value has changed, and register/unregister based on the new value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickmister can you give a reference for this.

Copy link
Contributor

@mickmister mickmister May 3, 2022

Choose a reason for hiding this comment

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

Look for usage of gitlab_refresh in the backend, and how it hooks into the frontend:

registry.registerWebSocketEventHandler(
`custom_${id}_gitlab_refresh`,
handleRefresh(store)

In order to unregister a component, you need to store the id returned from registry.registerLeftSidebarHeaderComponent etc, and pass the stored id to registry.unregisterComponent when necessary.

@hanzei hanzei removed their request for review April 27, 2022 22:19
@hanzei
Copy link
Collaborator

hanzei commented May 2, 2022

@sibasankarnayak Heads up that you need to merge master

@mattermod
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!

@hanzei hanzei closed this Nov 29, 2022
@hanzei hanzei mentioned this pull request Nov 29, 2022
@hanzei hanzei added Lifecycle/3:orphaned and removed 2: Dev Review Requires review by a core committer Lifecycle/1:stale 3: QA Review Requires review by a QA tester labels Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide Toolbar
4 participants