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

refactor(SceneByVariableRepeaterGrid): Pull out "groupBy" logic out of the class #109

Merged
merged 6 commits into from
Aug 2, 2024

Conversation

grafakus
Copy link
Contributor

@grafakus grafakus commented Aug 1, 2024

✨ Description

Related issue(s): is blocked by #108

This PR aims at making SceneByVariableRepeaterGrid more generic.

We achieve this:

  • by removing all the custom logic related to the "groupBy" variable from this class
  • by creating a new SceneLabelValuesGrid class, responsible for rendering the panels grid after the user has selected a "groupBy" value
  • by moving all the logic into the new class

We have a bit of duplicated code, but it's temporary. In the next PR, we'll start customizing SceneLabelValuesGrid so that the "Comparison" flow is easier to understand for the user (currently it's not obvious for users to understand how they can compare 2 label values).

📖 Summary of the changes

See diff tab for specific comments

🧪 How to test?

  • The build should pass
  • All panels grid should render and work as before

Copy link
Contributor

github-actions bot commented Aug 1, 2024

Unit test coverage

Lines Statements Branches Functions
Coverage: 12%
12.17% (470/3860) 9.61% (138/1435) 9.16% (108/1178)

@@ -36,15 +36,18 @@ interface SceneByVariableRepeaterGridState extends EmbeddedSceneState {
variableName: string;
items: GridItemData[];
headerActions: (item: GridItemData, items: GridItemData[]) => VizPanelState['headerActions'];
mapOptionToItem: (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decoupling: this new option allows the consumers of the class to customize how they build the grid items data.

value: option.value as string,
label: option.label,
queryRunnerParams: {
serviceName: option.value as string,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Way cleaner/more explicit than before

@@ -30,6 +30,18 @@ export class SceneExploreFavorites extends SceneObjectBase<SceneExploreFavorites
body: new SceneByVariableRepeaterGrid({
key: 'favorites-grid',
variableName: 'favorite',
mapOptionToItem: (option) => {
// see FavoritesDataSource.ts
const { index, value, panelType, queryRunnerParams } = JSON.parse(option.value as string);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here

@grafakus grafakus changed the base branch from main to feat/batch-of-improvements August 1, 2024 12:53
Base automatically changed from feat/batch-of-improvements to main August 2, 2024 09:52
Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

Nice refactor, I couldn't find any regressions with it 👏

LGTM!

@grafakus grafakus merged commit 3bedd48 into main Aug 2, 2024
4 of 5 checks passed
@grafakus grafakus deleted the feat/new-groupby-labels-grid branch August 2, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants