Skip to content
This repository was archived by the owner on Jul 22, 2025. It is now read-only.

Conversation

@martin-brennan
Copy link
Contributor

Copy link
Contributor

@markvanlan markvanlan left a comment

Choose a reason for hiding this comment

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

I am tempted to say it would be nice to have a test ensuring this component is registered for the key... but I hate those kinds of tests... So I won't say that. I did say it, but I won't actually say it though.

I'm surprised there is no other change here, I guess the report existed before but wasn't registered? Or maybe this is necessary, and the report would be loaded otherwise:?

@martin-brennan
Copy link
Contributor Author

@markvanlan before the refactor in that core PR, we used to do this:

@discourseComputed("currentMode")
  modeComponent(currentMode) {
    return `admin-report-${currentMode.replace(/_/g, "-")}`;
  }

But this kind of component lookup by filename kinda thing is not allowed in glimmer, so we have to switch to this:

get modeComponent() {
    const reportMode = this.currentMode.replace(/-/g, "_");
    switch (reportMode) {
      case REPORT_MODES.table:
        return AdminReportTable;
      case REPORT_MODES.inline_table:
        return AdminReportInlineTable;
      case REPORT_MODES.chart:
        return AdminReportChart;
      case REPORT_MODES.stacked_chart:
        return AdminReportStackedChart;
      case REPORT_MODES.stacked_line_chart:
        return AdminReportStackedLineChart;
      case REPORT_MODES.counters:
        return AdminReportCounters;
      case REPORT_MODES.radar:
        return AdminReportRadar;
      case REPORT_MODES.storage_stats:
        return AdminReportStorageStats;
      default:
        if (reportModeComponent(reportMode)) {
          return reportModeComponent(reportMode);
        }

        return null;
    }
  }

Registering the report component class for the plugin covers
that default fall through. I don't think there are any UI tests
covering this sentiment dashboard tab, otherwise
I would add to it to cover this change when the core PR merges

@martin-brennan martin-brennan merged commit c49b455 into main Jan 29, 2025
6 checks passed
@martin-brennan martin-brennan deleted the dev/use-new-core-admin-report-mode-register branch January 29, 2025 00:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants