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

Make resource logo sizing more consistent #1329

Merged
merged 4 commits into from
May 19, 2023

Conversation

kenxu95
Copy link
Contributor

@kenxu95 kenxu95 commented May 16, 2023

Describe your changes and why you are making these changes

This has been bothering me for a while. Our logos have been uploaded in a very adhoc fashion, so the sizing is significantly off sometimes.

This PR:

  1. Fixes a storybook bug with circular imports.
  2. Adds a story that puts all our resource logos side by side, with padding=1 so we can easily visualize the logos side by side.
  3. Uses the Spark-only logo instead of the wordy one.

I have the uploaded logo png files on my local filesystem. Once I merge this in, I'll update the public assets bucket with the updated ones on the next release.

This is what things used to look like:

Screen Shot 2023-05-16 at 3 09 27 PM Screen Shot 2023-05-16 at 3 09 15 PM

The Redshift, S3, Lambda, MariaDB, and MySQL logos were sized way too small. After adjusting, we get the following (MySQL and MariaDB still seem a little small, but that's just what their logos look like)

Screen Shot 2023-05-16 at 3 07 34 PM Screen Shot 2023-05-16 at 3 07 59 PM

I realize these all look bigger than the before, that's just cus my window size changed, just focus on the relative sizes.

Related issue number (if any)

ENG-2951

Loom demo (if any)

Checklist before requesting a review

  • I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
  • I have performed a self-review of my code.
  • I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
  • If this is a new feature, I have added unit tests and integration tests.
  • I have run the integration tests locally and they are passing.
  • I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
  • All features on the UI continue to work correctly.
  • Added one of the following CI labels:
    • run_integration_test: Runs integration tests
    • skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

@kenxu95 kenxu95 requested review from vsreekanti and agiron123 May 16, 2023 22:19
@vsreekanti
Copy link
Contributor

The mocks look good to me. Any idea what's going on with the crazy linter errors? If it's a known issue, I'm happy to approve this.

@agiron123
Copy link
Contributor

logos look good to me size wise, but I'd like to see the name of the integrations at the bottom.

@agiron123
Copy link
Contributor

If lint is still giving you trouble, you can try running on the file itself:

npx eslint ResourceLogoStory.stories.tsx --fix

@kenxu95 kenxu95 force-pushed the eng-2951-make-all-the-resource-logos-a-little branch from 0165da8 to 872c41c Compare May 19, 2023 01:08
@kenxu95 kenxu95 added the skip_integration_test Skips required integration test (only documentation/UI changes) label May 19, 2023
@kenxu95 kenxu95 merged commit f70e469 into main May 19, 2023
@kenxu95 kenxu95 deleted the eng-2951-make-all-the-resource-logos-a-little branch May 19, 2023 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip_integration_test Skips required integration test (only documentation/UI changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants