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

Schema tab: Fixed the header issue #5622

Conversation

Ankit-Keshari-Vituity
Copy link
Contributor

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). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions
Copy link

Unit Test Results (build & test)

499 tests  ±0   499 ✔️ ±0   8m 25s ⏱️ -17s
115 suites ±0       0 💤 ±0 
115 files   ±0       0 ±0 

Results for commit 21ba722. ± Comparison against base commit e9494f5.

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

Good stuff!

@@ -20,6 +20,10 @@ const NoSchema = styled(Empty)`
padding-top: 60px;
`;

const SchemaTableContainer = styled.div`
overflow: auto;
height: 100vh;
Copy link
Collaborator

Choose a reason for hiding this comment

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

it might be better to have this be height: 100%; but after checking locally I don't believe it really matters in this situation

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this mean that we are forcing schema tab height to the height of the browser window?

Copy link
Collaborator

@chriscollins3456 chriscollins3456 Aug 11, 2022

Choose a reason for hiding this comment

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

that is indeed what it's saying, but in this case it doesn't make a difference since this div can never grow higher than its parent. So in theory it's working the same as height: 100% but only in this situation. If there was no parent or if this was absolutely positioned it would fill the whole content and be the view height of the browser window.

All that being said I think here we actually just change it to be 100% instead of 100vh even though functionally there the same here (just better css practice)

@jjoyce0510 jjoyce0510 merged commit 6b819e9 into datahub-project:master Aug 11, 2022
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