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

Add Autocomplete icon #359

Merged
merged 5 commits into from
Sep 17, 2020
Merged

Add Autocomplete icon #359

merged 5 commits into from
Sep 17, 2020

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented Sep 7, 2020

Summary

Screenshot from 2020-09-07 18-25-56

The icon doesn't look optimal on a dark theme. @asaadmahmood I'm wondering what you think about this.

Ticket Link

Fixes #358

@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester 1: UX Review Requires review by a UX Designer labels Sep 7, 2020
@hanzei hanzei added this to the v2.0.0 milestone Sep 7, 2020
Comment on lines -66 to -67
DisplayName: "GitHub",
Description: "Integration with GitHub.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DisplayName and Description are not needed for plugins as they are not shown.

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2020

Codecov Report

Merging #359 into master will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #359      +/-   ##
==========================================
- Coverage   20.15%   20.10%   -0.05%     
==========================================
  Files          11       11              
  Lines        2615     2621       +6     
==========================================
  Hits          527      527              
- Misses       2050     2056       +6     
  Partials       38       38              
Impacted Files Coverage Δ
server/plugin/command.go 14.07% <0.00%> (-0.16%) ⬇️
server/plugin/configuration.go 28.12% <0.00%> (-2.91%) ⬇️

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 351e078...5cd2806. Read the comment docs.

@hanzei
Copy link
Contributor Author

hanzei commented Sep 7, 2020

Heads up: The code might be replaced by the one in mattermost/mattermost-plugin-api#71 if the PR gets accepted.

@asaadmahmood
Copy link
Contributor

@hanzei We should use the same github image as we use for the Github bot.
The Github bot image that we use has a white background behind it.

Screenshot 2020-09-08 at 12 34 51 PM

@hanzei
Copy link
Contributor Author

hanzei commented Sep 8, 2020

@asaadmahmood In the Marketplace modal we put a white background behind every icon. I was wondering we we want to do the same here and don't task the developer with providing the background.

@asaadmahmood
Copy link
Contributor

@hanzei We can do that, but we would still have to use a different image that has a padding built in. So that the image is not too close to the edges of the white background.
I would lean towards not having a white background, just in case people intentionally want to keep the bg transparent.

@hanzei
Copy link
Contributor Author

hanzei commented Sep 8, 2020

Do we have to have a different image? This is how it looks in the Marketplace with the same svg used as in this PR.
Screenshot from 2020-09-08 16-39-10

@asaadmahmood
Copy link
Contributor

@hanzei Here's the image in our Github bot avatar, we can use this.
image

@hanzei
Copy link
Contributor Author

hanzei commented Sep 8, 2020

As per request from @asaadmahmood I've updated the icon to use one with a white background:
Screenshot from 2020-09-08 17-43-51
Screenshot from 2020-09-08 17-43-37

@hanzei hanzei requested a review from larkox September 8, 2020 15:46
@hanzei
Copy link
Contributor Author

hanzei commented Sep 15, 2020

@asaadmahmood Does this look good to you?

Copy link
Contributor

@asaadmahmood asaadmahmood left a comment

Choose a reason for hiding this comment

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

Yup, looks good.

@hanzei hanzei removed the 1: UX Review Requires review by a UX Designer label Sep 15, 2020
@DHaussermann
Copy link

  • Icon always displays as expected
  • Checked icon looks good on all themes
  • Tested on desktop and various browsers

@hanzei This change is working but, something is odd about the unit tests. Can you please take a look?

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        4.118 s
Ran all test suites.
cd ./build/sync && /usr/local/go/bin/go test -v -race ./...
go: warning: "./..." matched no packages
no packages to test
make: *** [test] Error 1

@hanzei
Copy link
Contributor Author

hanzei commented Sep 17, 2020

@DHaussermann #363 will fix the failure in make test. That issue is unrelated to this PR.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and Passed
Only one issue was found in testing. It is unrelated and will be addressed separately.

LGTM!

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Sep 17, 2020
@hanzei
Copy link
Contributor Author

hanzei commented Sep 17, 2020

Waiting for #363 to be merged first.

@hanzei hanzei added the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Sep 17, 2020
@hanzei hanzei removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Sep 17, 2020
@hanzei hanzei merged commit 95615ad into master Sep 17, 2020
@hanzei hanzei deleted the 358_icon branch September 17, 2020 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add icon to autocomplete data
6 participants