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

Metrics filter layout #1282

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Metrics filter layout #1282

merged 2 commits into from
Dec 8, 2023

Conversation

tlmii
Copy link
Member

@tlmii tlmii commented Dec 7, 2023

Resolves #1258

Uses flexbox layout rather than the @media and max-width method.

This also allows it to wrap when you move the slider, which didn't work before (it only wrapped if you changed the browser window size).

One thing I changed here is that I got rid of the left margin for the main header and the filter (when wrapped). You can see this below. There's no CSS-only way to detect that an item has wrapped to another line in flexbox, so I couldn't easily conditionally add it to the filters. I think it still looks fine, but we could do a general UX pass on the metrics page overall separate from this issue if we wanted.

Before:
MetricsFilterWrap-Before

After:
MetricsFilterWrap-After

Note: ignore whitespace to see the actual changes in ChartContainer.razor.

@@ -2,4 +2,4 @@
@namespace Aspire.Dashboard.Components
@inject IStringLocalizer<ControlsStrings> Loc

<div id="plotly-chart-container" style="width:650px; height:400px"></div>
<div id="plotly-chart-container" style="width:650px; height:450px"></div>
Copy link
Member Author

Choose a reason for hiding this comment

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

The plot was actually rendering larger than 400px so when things wrapped there would be overlap between the bottom of the chart and the filters.

@JamesNK
Copy link
Member

JamesNK commented Dec 7, 2023

One regression I see is the wrapped filters is displayed right next to the splitter. Previously it was indented to line up with the Y axis of the chart.

Hacky way to fix:

  • Add padding-left: 40px; to metrics-chart-container
  • Add margin-left: -40px; to plotly-chart-container

@tlmii
Copy link
Member Author

tlmii commented Dec 8, 2023

One regression I see is the wrapped filters is displayed right next to the splitter. Previously it was indented to line up with the Y axis of the chart.

Hacky way to fix:

  • Add padding-left: 40px; to metrics-chart-container
  • Add margin-left: -40px; to plotly-chart-container

This was semi-intentional. Part of it was what I said above about there being no good way to detect the wrapping, so the only way around it was hackiness like above. The other part was that to my eye the alignment never seemed quite right. There were always too many possible "left edge" of the chart area (left edge of the x-axis labels, left edge of the plot lines, left edge of the chart area itself and left edge of the legend). They're all so close together but not lined up and so trying to line up the header didn't seem worthwhile:

image

So I just lined up both headers (the main one at the top and the filters) along the left edge:

image

Moving the headers also came from just looking around at the few other charting examples I could find and not seeing ones where the chart header was aligned with the Y axis (it seemed to always be aligned with the left edge of the container or centered).

Your workaround does work though if we want to keep those left margins.

With margins:
image

without:
image

My eye doesn't really like either one for some reason, but I don't have a better solution at the moment (I think what I'd prefer is for the chart and headers to be centered and the filters either be hidden by default or always below it or something, but I'd need a lot more experimentation.

Bottom line after all that text and all those pictures is I went ahead and used your suggestion to give the headers back the left margins and we can deal with whether or not its the right overall layout elsewhere.

@tlmii tlmii enabled auto-merge (squash) December 8, 2023 07:01
@tlmii tlmii merged commit 2fe7fd2 into dotnet:main Dec 8, 2023
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metrics: Improve wrapping metrics filters UI on low resolution screens
2 participants