-
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
[Infra UI] Implement telemetry for the asset details flyout #163078
Changes from 8 commits
d70461f
48b947c
1e725fe
3f36690
367638a
4cca4c7
7fffa3c
c6e0a4f
10fcd98
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,4 +5,5 @@ | |
* 2.0. | ||
*/ | ||
|
||
export const ASSET_DETAILS_FLYOUT_COMPONENT_NAME = 'infraAssetDetailsFlyout'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to have a different name in case it's open as page? Maybe I am missing something but I don't see a different There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. the name will be different. Probably |
||
export const METRIC_CHART_HEIGHT = 300; |
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.
I realized that this will conflict with my changes in my PR where I use
showInFlyout
as a boolean. I can adapt that once this PR is merged. It is all good, I am just curious now if it is somehow used in the telemetry object or not yet (I don't see the mode when I check it just want to be sure that I am not missing something)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.
renderMode
will only be used to determine thecomponentName
attribute. I changed it because this way seems more explanatory to differentiate between render mode:page
vsflyout
. Not sure if you agree.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.
Ok, got it! Yeah, it makes sense, we can also extend with other modes that way! I used the boolean to have the "default" view and "flyout" view so the consumer should add a prop only in case a flyout is needed, but your change is good, thanks for explaining 👍