Conversation
There was a problem hiding this comment.
Code Review
This pull request promotes the "Inspector V2" to the default inspector by renaming directories, updating imports, and removing the inspectorV2 feature flag across the codebase. It also cleans up related analytics metrics, documentation imports, and integration test goldens. Feedback focuses on the DiagnosticsNodeDescription widget, where the new logic for preserving font sizes during style merges is flagged as redundant and potentially incorrect. It is recommended to simplify these merges by using copyWith(fontSize: null) on the incoming styles to prevent them from overwriting the existing font size.
| textStyle = textStyle.merge(theme.subtleTextStyle); | ||
| final fontSize = textStyle.fontSize; | ||
| if (fontSize != null) { | ||
| textStyle = textStyle.copyWith(fontSize: fontSize); | ||
| } |
There was a problem hiding this comment.
[CONCERN] The logic to preserve the font size is redundant and incorrect here. fontSize is captured after the merge operation (line 306), meaning it will capture the font size from theme.subtleTextStyle if it exists, and then re-apply it. This differs from the implementation at line 405 where it is captured before the merge. A cleaner and more robust approach to ensure the font size is not overwritten by subtleTextStyle is to use copyWith(fontSize: null) on the style being merged.
textStyle = textStyle.merge(
theme.subtleTextStyle.copyWith(fontSize: null),
);| final fontSize = textStyle.fontSize; | ||
| textStyle = textStyle.merge(theme.subtleTextStyle); | ||
| if (fontSize != null) { | ||
| textStyle = textStyle.copyWith(fontSize: fontSize); | ||
| } |
There was a problem hiding this comment.
[CONCERN] This logic can be simplified by using copyWith(fontSize: null) on the style being merged. This prevents subtleTextStyle from overwriting the current font size during the merge, which is more concise than capturing and re-applying the font size manually.
textStyle = textStyle.merge(
theme.subtleTextStyle.copyWith(fontSize: null),
);| @@ -201,7 +201,7 @@ class DevToolsAnalyticsEvent { | |||
| : null, | |||
| // [InspectorScreenMetrics] | |||
| isV2Inspector: screenMetrics is InspectorScreenMetrics | |||
There was a problem hiding this comment.
Hmm does something need to be done here? Removing isV2Inspector?
Closes #7860
Work towards #9563
Follow up to #9782
This PR:
inspector_v2->inspectorNote: The only exception is our analytics. I kept the inspector V2 analytics dimension, which is
truefor any inspector usage