Skip to content

Conversation

jake-bassett
Copy link
Contributor

Description

After fixing the Safari issues with flexbox, the Explorer table was expanding beyond the page limits. This PR fixes that and a few other small styling issues with table in explorer.

Testing

Manual testing of this table and tables in other parts of the product, including entity lists, events, and api def.

@jake-bassett jake-bassett requested a review from a team as a code owner January 7, 2021 22:04
@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #485 (e8e6c59) into main (6c83f78) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #485   +/-   ##
=======================================
  Coverage   85.68%   85.68%           
=======================================
  Files         749      749           
  Lines       15321    15321           
  Branches     1814     1814           
=======================================
  Hits        13128    13128           
  Misses       2162     2162           
  Partials       31       31           
Impacted Files Coverage Δ
...rvability/src/pages/explorer/explorer.component.ts 100.00% <ø> (ø)

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 6c83f78...e8e6c59. Read the comment docs.

@github-actions

This comment has been minimized.


.table {
flex: 1 1;
width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

this scares me - shouldn't the table be sized not to overflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, its odd. But this is the overflow for the table-widget-renderer. There is another overflow:auto in table itself that usually does the scrolling, but in explorer due to the way the page flexes, it is required here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to hidden, because that's really more accurate since it isn't actually doing the scrolling.

Copy link
Contributor

Choose a reason for hiding this comment

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

I pulled it locally and chromium looks fine, but safari doesn't -
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, using min-height: 0; instead of the overflow seems to have the same effect in chromium (but also, unfortunately the same effect in safari - perhaps that's a pre-existing issue?)

Copy link
Contributor Author

@jake-bassett jake-bassett Jan 7, 2021

Choose a reason for hiding this comment

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

After looking a bit, I'm guessing this must have been existing for a while. We just don't get Safari traffic so it hasn't been reported.

Can we get this fix in so at least get things fixed on Chrome and FF? I'll follow up with a fix for Safari.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep - do you think it makes min-height instead of overflow here? I was feeling it was better because overflow (auto or hidden) is describing behavior we don't actually want - it should never overflow by design. Min-height works because the issue that I was seeing is the default of min-height:auto in flex col layout was preventing shrinking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, agreed. Let me test that change in the Safari fix and use min-height instead if it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think I found a solution that works for Safari. Just going to tack it on to this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@jake-bassett jake-bassett merged commit 5aa9a4f into main Jan 8, 2021
@jake-bassett jake-bassett deleted the fix-explorer-table-scrolling branch January 8, 2021 00:12
@github-actions
Copy link

github-actions bot commented Jan 8, 2021

Unit Test Results

    4 files  ±0  230 suites  ±0   12m 49s ⏱️ -31s
797 tests ±0  797 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
801 runs  ±0  801 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 5aa9a4f. ± Comparison against base commit 6c83f78.

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.

2 participants