-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Overview heights and loading states #83360
Conversation
* Set the chart height to fill the whole container * Remove the initial loading spinner for the tables and always show the progress bar * Make the last seen column on the errors table a bit wider so it doesn't wrap * Make a `ServiceOverviewTable` component that pins the pagination to the bottom of the panel * Show the loading spinner on charts when doing updates
5db1a2d
to
49d060e
Compare
Pinging @elastic/apm-ui (Team:apm) |
@@ -211,11 +211,7 @@ export function TransactionDistribution({ | |||
/> | |||
</h5> | |||
</EuiTitle> | |||
<ChartContainer | |||
height={unit * 10} | |||
hasData={!!(distribution && !distribution.noHits)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting rid of hasData
👍
* The height for a table on the overview page. Is the height of a 5-row basic | ||
* table. | ||
*/ | ||
const tableHeight = 298; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause problems as soon as Eui changes anything related to the height of the row (padding, margin, border etc), right. Would be great if we ca make it dynamic eg:
const tableHeight = 298; | |
const tableHeight = 5 * EuiTable.rowHeight; |
Probably not a variable that's exposed to but might be worth asking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The row doesn't have a height set explicitly, the actual table height is calculated by:
s = height of the sparkline chart (the tallest element in the row) = 24px
c = the table cell padding = 8px * 2
b = the table row borders and padding = 2px * 2
n = the number of rows = 5
h = height of the heading = 34px
p = height of the pagination = 40px
So it's ((s + c + b) * 5) + h + p = 298px
If we or EUI were to change any of these it would just make it so there's a bit of extra whitespace at the bottom of the chart or the table, so I think it's safe to leave it for now.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
* Overview heights and loading states * Set the chart height to fill the whole container * Remove the initial loading spinner for the tables and always show the progress bar * Make the last seen column on the errors table a bit wider so it doesn't wrap * Make a `ServiceOverviewTable` component that pins the pagination to the bottom of the panel * Show the loading spinner on charts when doing updates
* Set the chart height to fill the whole container * Remove the initial loading spinner for the tables and always show the progress bar * Make the last seen column on the errors table a bit wider so it doesn't wrap * Make a `ServiceOverviewTable` component that pins the pagination to the bottom of the panel * Show the loading spinner on charts when doing updates
ServiceOverviewTable
component that pins the pagination to the bottom of the panel