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

Adjust connector icons #20547

Merged
merged 9 commits into from
Jan 10, 2023
Merged

Adjust connector icons #20547

merged 9 commits into from
Jan 10, 2023

Conversation

timroes
Copy link
Collaborator

@timroes timroes commented Dec 15, 2022

What

Adjust all our connector icons with ones Nico adjusted (to square them and add correct padding). Also I minimized all SVGs (using svgo) of connector icons to make sure we're not transferring more bytes than needed for those icons.

This also adds a new gradle task to airbyte-config that will validate, that the icons that are used inside the seed files actually exist as well as making sure that no orphaned icons are laying around in the icons folder that aren't used anywhere.

@timroes timroes requested a review from a team December 15, 2022 22:13
Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

Two comments:

  • It looks like some of these icons which are not square-shaped have been made a bit larger vertically or horizontally than the square icons (maybe to make up for the fact that they are skinnier in one of those directions?), but this makes them look noticeably larger than the squarish icons. For example, TikTok and Zendesk Chat in this screenshot, compared to Stripe and Twilio. Do we want to try to make all of these look more similar in size, or is this acceptable?
    Screen Shot 2022-12-15 at 2 47 21 PM
  • The placeholder "wrench/screwdriver" icon looks off-center when compared with the other icons. It seems to be positioned more to the left than everything else:
    Screen Shot 2022-12-15 at 2 51 52 PM

@timroes
Copy link
Collaborator Author

timroes commented Dec 16, 2022

For the first point, @Upmitt can you have a look at that.

For the second: yes that problem though is unrelated to this change and already existing on master. Did not to plan changing this with this PR.

@Upmitt
Copy link
Contributor

Upmitt commented Dec 20, 2022

thank you @lmossman - you're right, I updated a few icons :) - .zip sent to @timroes

@timroes timroes requested a review from a team as a code owner December 20, 2022 18:23
@timroes timroes temporarily deployed to more-secrets December 20, 2022 18:25 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 20, 2022 18:25 — with GitHub Actions Inactive
@timroes
Copy link
Collaborator Author

timroes commented Dec 20, 2022

Failure to validate that the new gradle check is working: https://github.com/airbytehq/airbyte/actions/runs/3743133413/jobs/6354963833#step:18:8067

@timroes timroes temporarily deployed to more-secrets December 21, 2022 17:21 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 21, 2022 17:29 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 21, 2022 17:29 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 21, 2022 20:23 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 21, 2022 20:25 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 21, 2022 20:26 — with GitHub Actions Inactive
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/server labels Dec 21, 2022
@timroes timroes temporarily deployed to more-secrets December 21, 2022 21:39 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets December 21, 2022 21:40 — with GitHub Actions Inactive
@timroes timroes requested a review from lmossman January 9, 2023 12:39
@timroes timroes temporarily deployed to more-secrets January 9, 2023 12:41 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets January 9, 2023 12:41 — with GitHub Actions Inactive
@timroes timroes removed the request for review from a team January 9, 2023 12:54
@timroes timroes temporarily deployed to more-secrets January 9, 2023 12:55 — with GitHub Actions Inactive
@timroes timroes temporarily deployed to more-secrets January 9, 2023 12:56 — with GitHub Actions Inactive
@timroes timroes requested a review from flash1293 January 9, 2023 17:29
Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

The S3 icon was the only one that surprised me when I was testing locally. I haven't seen that icon used for S3 before -- the old S3 icon that we currently have is the one that I recognize as S3. But maybe I'm just not aware of an icon switch that amazon has done?

@Upmitt might be worth double checking that the s3 icon is correct.

Otherwise these icons look good to me. Tested locally by rebuilding the airbyte-server on this branch and it looks good

@timroes timroes merged commit 4556b61 into master Jan 10, 2023
@timroes timroes deleted the tim/connector-icons branch January 10, 2023 11:30
jbfbell pushed a commit that referenced this pull request Jan 13, 2023
* Adjust connector icons

* Update icons

* Add missing icons

* Remove missing icon from spec

* Add trailing newline

* Fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform area/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants