fix - adapting theme switcher for echarts#2526
Conversation
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a snapshot-based mechanism to the ThemeSwitcher to prevent redundant theme change events. While the implementation improves efficiency, the snapshot logic should be expanded to include the colorSchema property to ensure all relevant changes are captured. Additionally, the PR needs to be updated to comply with project standards, specifically regarding the title format, the inclusion of a changeset for consumer-relevant changes, and the addition of missing unit tests as indicated in the checklist.
| private getSnapshot(): string { | ||
| return `${this.getTheme()}-${this.getMode()}`; | ||
| } | ||
|
|
||
| private emitThemeChange(isMediaChange: boolean) { | ||
| this.themeChanged.emit({ | ||
| const snapshot = this.getSnapshot(); | ||
| if (snapshot === this.lastEmittedSnapshot && !isMediaChange) return; | ||
| this.lastEmittedSnapshot = snapshot; | ||
|
|
||
| const detail: ThemeChangeEventDetail = { | ||
| theme: this.getTheme(), | ||
| colorSchema: this.getColorSchema(), | ||
| mode: this.getMode(), | ||
| isMediaChange, | ||
| }); | ||
| }; | ||
| this.themeChanged.emit(detail); | ||
| } |
There was a problem hiding this comment.
The current snapshot logic in getSnapshot only considers theme and mode. However, ThemeChangeEventDetail also includes the colorSchema property. If the colorSchema changes (e.g., from 'system' to 'dark') while the resolved mode remains the same (because the system was already dark), the event will not be emitted. This results in listeners receiving stale information about the colorSchema.
Additionally, the emitThemeChange method performs multiple redundant calls to getTheme, getColorSchema, and getMode. I suggest refactoring this to capture the values once and include colorSchema in the snapshot.
private emitThemeChange(isMediaChange: boolean) {
const theme = this.getTheme();
const colorSchema = this.getColorSchema();
const mode = this.getMode();
const snapshot = theme + "-" + colorSchema + "-" + mode;
if (snapshot === this.lastEmittedSnapshot && !isMediaChange) return;
this.lastEmittedSnapshot = snapshot;
this.themeChanged.emit({
theme,
colorSchema,
mode,
isMediaChange,
});
}| class ThemeSwitcher { | ||
| private mutationObserver?: MutationObserver; | ||
| private mediaQueryList?: MediaQueryList; | ||
| private lastEmittedSnapshot?: string; |
There was a problem hiding this comment.
The pull request title 'fix - adapting theme switcher for echarts' does not adhere to the required format: '[optional ]: '. Please update it to follow the convention (e.g., 'fix(core): adapting theme switcher for echarts').
References
- Follow the rule [optional ]: for pull request titles. (link)
| class ThemeSwitcher { | ||
| private mutationObserver?: MutationObserver; | ||
| private mediaQueryList?: MediaQueryList; | ||
| private lastEmittedSnapshot?: string; |
There was a problem hiding this comment.
This pull request introduces consumer-relevant changes to theme emission behavior. According to the style guide, a changeset should be included for such changes to ensure proper release management and documentation.
References
- Treat a missing changeset as a review issue when a pull request includes user-facing or consumer-relevant changes. (link)
| this.setColorSchema(colorSchema); | ||
| document.documentElement.dataset.ixColorSchema = | ||
| colorSchema ?? this.getColorSchema(); | ||
| this.emitThemeChange(false); |
There was a problem hiding this comment.
The PR checklist indicates that unit tests were added or updated, but no changes to test files are present in this diff. Automated tests are essential to verify the fix for rapid toggling and to prevent regressions in theme emission logic.
References
- Focus on missing validation and ensure adequate testing for changes.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the ThemeSwitcher by implementing snapshot-based change detection to prevent redundant emissions and introducing a new global ix-theme-change DOM event. Review feedback highlights several style guide violations: the PR title needs to follow the standard format, a changeset is required for the new public event and behavior changes, and the new event must be documented with a @SInCE tag.
| class ThemeSwitcher { | ||
| private mutationObserver?: MutationObserver; | ||
| private mediaQueryList?: MediaQueryList; | ||
| private lastEmittedSnapshot?: string; |
There was a problem hiding this comment.
The pull request title fix - adapting theme switcher for echarts does not follow the required format. According to the repository style guide, it should follow the pattern <type>[optional <scope>]: <description>. Please update the title to use a colon (e.g., fix: adapting theme switcher for echarts).
References
- Pull request titles must follow the rule [optional ]: . (link)
| class ThemeSwitcher { | ||
| private mutationObserver?: MutationObserver; | ||
| private mediaQueryList?: MediaQueryList; | ||
| private lastEmittedSnapshot?: string; |
There was a problem hiding this comment.
This pull request introduces consumer-relevant changes, including a new global DOM event ix-theme-change and a change to synchronous theme change emissions. According to the repository style guide, a changeset is required for such changes. Please add a changeset file describing these updates.
References
- A changeset is required for public API updates, component behavior changes, styling or theming changes, accessibility changes, and bug fixes that materially affect users. (link)
| this.themeChanged.emit(detail); | ||
|
|
||
| if (typeof document !== 'undefined') { | ||
| document.dispatchEvent(new CustomEvent('ix-theme-change', { detail })); |
There was a problem hiding this comment.
The new global DOM event ix-theme-change should be documented with a @since tag indicating the version in which it was introduced, as per the repository style guide for new events. Since this is a DOM event dispatched on the document, consider adding the documentation in the class JSDoc or near the dispatch point.
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
2 participants
💡 What is the current behavior?
Theme in echarts when toggled using ixmenu doesnt always take effect
GitHub Issue Number: NA / ix-4159
🆕 What is the new behavior?
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support