MM-40214: Add App Bar icon

mattermost-community / mattermost-plugin-jitsi

Jitsi plugin for Mattermost :electric_plug:

Apache License 2.0

193 stars 89 forks source link

MM-40214: Add App Bar icon #214

Closed agarciamontoro closed 2 years ago

agarciamontoro commented 2 years ago

Summary

This PR adds a new icon explicitly for the App Bar, so that when it is enabled, the proper icon is shown. The logic for rendering the channel header icon or the App Bar one is in the webapp, not here.

I needed to do a couple of things to build this locally:

Use node 14.
Run go get -u golang.org/x/sys, which is added in the first commit.
Use the fix from this open PR. I have not added that as a commit here to avoid conflicts in that other PR, but if you're building locally, note that you may need it.

I also added a commit to fix CI, adding the --verbose solution we've used in other plugins to avoid CI from timing out.

Demo

https://user-images.githubusercontent.com/3924815/172824065-ad4e1010-f263-40c4-8a96-fcdeb327f673.mp4

Ticket Link

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

agarciamontoro commented 2 years ago

Note that we're aiming to release a new version of the plugin with this PR before the release of v7.0 on June 15 (Wednesday next week).

agarciamontoro commented 2 years ago

Swapping Daniel with Lev because Daniel's out today and tomorrow.

agarciamontoro commented 2 years ago

@mickmister, I pushed a change to the estlint rules just 30 seconds after you reviewed :joy: I think it's sensible, since otherwise the following block returns an error:

const action = (channel: Channel) => {
    store.dispatch(startMeeting(channel.id));
};

But I wanted you to take a look. That's why I'm asking for a re-review :)

agarciamontoro commented 2 years ago

Heh, CI is failing because of the error solved in #211. @DHaussermann, if you can take a look at that PR before this one so I can merge that fix in my branch, that'd be awesome. Let me know if you need anything from me, please!

DHaussermann commented 2 years ago

@agarciamontoro this is good to merge but depending on when a release with this is needed, we may need to package a separate release with this change.

agarciamontoro commented 2 years ago

@DHaussermann, thank you very much!

this is good to merge but depending on when a release with this is needed, we may need to package a separate release with this change.

Got it! As this PR depends on the build fix implemented in https://github.com/mattermost/mattermost-plugin-jitsi/pull/211, I will add that fix here, merge this PR and fix the conflicts in #211 so you guys can merge it whenever you want. Then I can create a new release-2.0 branch with only this PR on top of v2.0.0, release a new v2.1.0 from there and publish it in the Marketplace. Then you can follow your usual release schedule for the next ones, if that makes sense.