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

Update table every second, reuse existing metric values if possible #3370

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

adamint
Copy link
Member

@adamint adamint commented Apr 3, 2024

We can associate rows with the underlying data's time range so that focus is not lost when the data is updated, if keyboard focus is active inside the table.

Fixes #3363

Animation

Copy link
Member

@tlmii tlmii left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with the metrics code, but this change seems reasonable for the purpose.

My only question, for future proofing, would be: Are we sure that re-using the same object will always work? That just feels a little "implementation detail". I know there's ItemKey available on the data grid. Does that achieve a similar result, and if so, might that be more reliable?

Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

I'd like some comments about why code is needed.

@dotnet-policy-service dotnet-policy-service bot added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Apr 16, 2024
@adamint
Copy link
Member Author

adamint commented Apr 16, 2024

My only question, for future proofing, would be: Are we sure that re-using the same object will always work? That just feels a little "implementation detail". I know there's ItemKey available on the data grid. Does that achieve a similar result, and if so, might that be more reliable?

Yes, that achieves the same result! I like that approach a lot more... did not realize ItemKey existed. I've updated the code to use that isntead

@adamint adamint requested review from JamesNK and tlmii April 16, 2024 15:38
@JamesNK
Copy link
Member

JamesNK commented Apr 17, 2024

It fails with a runtime error...

image

@JamesNK JamesNK enabled auto-merge (squash) April 17, 2024 03:58
@JamesNK JamesNK merged commit dab5352 into dotnet:main Apr 17, 2024
8 checks passed
@kvenkatrajan
Copy link
Member

@adamint please backport for 8.0 - might be late for preview 6 - confirm.

@adamint
Copy link
Member Author

adamint commented Apr 17, 2024

/backport to release/8.0

Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/aspire/actions/runs/8725980474

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants