Skip to content

Conversation

jake-bassett
Copy link
Contributor

Description

Add spacing between title and table content if a title is provided.

@jake-bassett jake-bassett requested a review from a team as a code owner March 24, 2021 17:34
@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #718 (65769af) into main (6ebd2c9) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #718      +/-   ##
==========================================
- Coverage   85.16%   85.15%   -0.01%     
==========================================
  Files         784      784              
  Lines       16066    16064       -2     
  Branches     1997     1997              
==========================================
- Hits        13682    13680       -2     
  Misses       2351     2351              
  Partials       33       33              
Impacted Files Coverage Δ
...shboards/src/widgets/header/widget-header.model.ts 100.00% <ø> (ø)
...dashboard/widgets/table/table-widget-base.model.ts 78.37% <ø> (-0.57%) ⬇️
...d/widgets/table/table-widget-renderer.component.ts 45.27% <ø> (ø)
...kend-detail/overview/backend-overview.component.ts 40.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 6ebd2c9...65769af. Read the comment docs.

@github-actions

This comment has been minimized.

changeDetection: ChangeDetectionStrategy.OnPush,
template: `
<ht-titled-content
class="table-title"
Copy link
Contributor

Choose a reason for hiding this comment

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

I see there's already a [ngClass]="{ 'header-margin': this.model.header?.topMargin }" - is that the same thing? If the two don't have different purposes, can we resolve to one way of doing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The header margin is for the table controls header. The new one is for the title that sits above that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's a bug though - the header model where this top margin is defined is used for the title, not table controls. I'm guessing when table controls were added, it created an unintentional separation between the title and where that margin was applied. If you look where this is used today, it's only in one spot - backend overview, on a table that doesn't have controls (which is likely why the bug was never caught - it was still rendering the same).

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Mar 24, 2021

Choose a reason for hiding this comment

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

So the places (HT home dash) where the header is used without topMargin being set just look bad - they should have this property set. If we want to remove that property entirely and automatically add the appropriate padding if title is set, I think that works.

image

We just want to make sure we're not double padding it if that property is set, so we should do one or the other.

@github-actions

This comment has been minimized.

@jake-bassett jake-bassett merged commit 90d31a5 into main Mar 24, 2021
@jake-bassett jake-bassett deleted the fix-title-spacing-table-widget branch March 24, 2021 23:30
@github-actions
Copy link

Unit Test Results

    4 files  ±0  245 suites  ±0   14m 46s ⏱️ -38s
878 tests ±0  878 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
882 runs  ±0  882 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 90d31a5. ± Comparison against base commit 6ebd2c9.

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