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

feat(react): add configurable logo aspect ratio #2589

Merged
merged 2 commits into from
May 21, 2021

Conversation

martha
Copy link
Contributor

@martha martha commented May 20, 2021

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

Copy link
Contributor

@gabe-lyons gabe-lyons left a comment

Choose a reason for hiding this comment

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

Great! Thanks for this contribution @martha. Could I ask you to also update src/app/home/HomePageHeader.tsx (the other location the logo image is rendered) and make sure it also respects the aspect ratio?

@martha martha marked this pull request as ready for review May 20, 2021 18:00
@martha
Copy link
Contributor Author

martha commented May 20, 2021

@gabe-lyons good suggestion! The logo wasn't looking distorted on the home page, which is why I didn't update that file. I noticed that in src/app/home/HomePageHeader.tsx it specifies only a width, no height, for the logo image.

That makes me wonder whether the whole aspect ratio approach I outlined here is overthinking. Should I just modify SearchHeader.tsx to specify a height (32px) but no width?

@martha
Copy link
Contributor Author

martha commented May 20, 2021

Homepage before
homepage_before

Homepage after this PR - it's the same
homepage_after

Search page before - distorted
search_before

Search page after - distortion fixed
search_after

edit - fixed image order

@gabe-lyons
Copy link
Contributor

@martha I like your suggestion to fix the height of the search icon and let the width flex- this keeps the theme simpler. Let's go with that 👍

@martha
Copy link
Contributor Author

martha commented May 20, 2021

Sure! Here's an updated screen shot after the new commit.
Screen Shot 2021-05-20 at 2 17 04 PM

Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @martha :)

@shirshanka shirshanka merged commit b56ec27 into datahub-project:master May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants