-
Notifications
You must be signed in to change notification settings - Fork 90
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
MM-40214: Add App Bar icon #214
Conversation
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). |
Swapping Daniel with Lev because Daniel's out today and tomorrow. |
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 Alejandro!
@mickmister, I pushed a change to the estlint rules just 30 seconds after you reviewed 😂 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 :) |
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! |
@@ -11,4 +11,5 @@ require ( | |||
github.com/nicksnyder/go-i18n/v2 v2.0.3 | |||
github.com/pkg/errors v0.9.1 | |||
github.com/stretchr/testify v1.6.1 | |||
golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a // indirect |
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.
go mod tidy
does not get rid of this?
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.
Nope, I just tried.
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 passed
- Icon is registered on the App Bar
- Icon will fail-over to channel header when the App Bar is disabled
- Icon is functional
- Icon tooltip is correct
- Icon has good contrast in all themes
LGTM!
Sorry for the delay on this @agarciamontoro
@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. |
@DHaussermann, thank you very much!
Got it! As this PR depends on the build fix implemented in #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 |
* go get -u golang.org/x/sys * Fix CI * Add App Bar icon * Fix errors found by linter * Allow arrow functions in eslint's func-style rule * fix type mismatch at redux Provider Co-authored-by: Ngoan Tran <[email protected]>
* go get -u golang.org/x/sys * Fix CI * Add App Bar icon * Fix errors found by linter * Allow arrow functions in eslint's func-style rule * fix type mismatch at redux Provider Co-authored-by: Ngoan Tran <[email protected]>
* react-dom_webpack-external (#224) * bump version 2.0.1 * MM-40214: Add App Bar icon (#214) * go get -u golang.org/x/sys * Fix CI * Add App Bar icon * Fix errors found by linter * Allow arrow functions in eslint's func-style rule * fix type mismatch at redux Provider Co-authored-by: Ngoan Tran <[email protected]> * go mod tidy * remove npm i --verbose * Fix linter errors (#227) * lint Co-authored-by: Alejandro García Montoro <[email protected]> Co-authored-by: Ngoan Tran <[email protected]> Co-authored-by: Ben Schumacher <[email protected]>
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:
go get -u golang.org/x/sys
, which is added in the first commit.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
jitsi.mp4
Ticket Link
https://mattermost.atlassian.net/browse/MM-40214