crspeller / mattermost-plugin-channel-notes

A Mattermost plugin that extends channel functionality with notes.
Apache License 2.0
27 stars 5 forks source link

[MM-40214] Implement App Bar component #4

Closed mickmister closed 2 years ago

mickmister commented 2 years ago

Summary

This PR makes it so the plugin registers its own component to the App Bar. The core webapp makes it so the component is only mounted if the AppBarEnabled feature flag is on. This logic is only in the core webapp, and not in the plugin's source.

The plugin simultaneously registers its usual channel header component, and App Bar component. This is so the webapp can show one or the other when the App Bar feature flag is enabled or disabled. The plugin then doesn't need to watch this feature flag, and the webapp can manage the effects of the flag.

Ticket Link

Fixes https://mattermost.atlassian.net/browse/MM-40214

dipak-demansol commented 2 years ago

@mickmister when the app-bar is enable then Note plugin have issue, by click on the note icon i can't see any message or button to add data, i got the blank page, if the app-bar is disable then the same build is working properly, pls see the video

https://user-images.githubusercontent.com/89907719/168976412-2703761d-90b4-495b-a258-41d31fe52bc5.mov

DHaussermann commented 2 years ago

@mickmister I know you've opened this icon PR and this may well be unrelated but, I can now see the issue mentioned above.... The icon has moved but the notes app running v6.7.0 however I'm having setup issues isolating if it's related to app bar in any way. Based on the screen in the comment above it seems like it's not. image

Edit:Also - I think it's expected for v6.7.0 that the sorting of icons is not yet ideal

agarciamontoro commented 2 years ago

@DHaussermann, that bug is fixed in https://github.com/crspeller/mattermost-plugin-channel-notes/pull/5. I asked @crspeller to review that one today so we can merge master into this branch and test again.

agarciamontoro commented 2 years ago

Oh, and it is definitely related to the App Bar icon! Actually, to the channel header icon not being mounted, which is a consequence of rendering the App Bar icon. The problem was that the channel header button component contains logic needed for the correct functioning of the plugin.

agarciamontoro commented 2 years ago

The CI failures here are permissions-related, and we cannot fix them until Christopher's back, two weeks from now. I have access to the repo, but not to CircleCI, and it seems we need to create a new deploy key as per https://support.circleci.com/hc/en-us/articles/360021666393-How-to-stop-building-by-manually-removing-the-CircleCI-webhook-and-deploy-key-from-your-GitHub-repository, which I don't have permissions for.

As we need this before the release of v7.0 on June 15 (next Wednesday), we'll merge after @DHaussermann tests it works as expected.

DHaussermann commented 2 years ago

Merge from master resolved the issue above.

Tested and passed

LGTM!

DHaussermann commented 2 years ago

As per @agarciamontoro CI failures here are permission related and can't be immediately resolved.

Please merge.

agarciamontoro commented 2 years ago

Thank you, @DHaussermann! :tada: