fix: resetting on global typography should only affect the current de… #3621
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughDevice-aware typography reset and comparison logic updated in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Global Typography UI
participant Logic as Typography Logic
participant Store as Settings Store
rect rgba(235,245,255,0.7)
note over UI,Logic: Determine if Reset button should show
User->>UI: View selector (Desktop/Mobile/Tablet)
UI->>Logic: getIsAllowReset(selector, device)
Logic->>Logic: compute typographyStyleOnCurrentDevice
Logic->>Store: fetch current typography + font pair
Logic->>Logic: compare device-specific keys and fontFamily
Logic-->>UI: allowReset (true/false)
end
alt User clicks Reset
UI->>Logic: resetTypography(selector, device)
Logic->>Store: read current per-selector styles
Logic->>Logic: getDefaultFontFamily(fontPair)
Logic->>Logic: clear device keys (fontSize, unit, lineHeight, letterSpacing)
Logic->>Logic: clean empty keys
Logic->>Store: apply updated styles
Store-->>UI: updated state
else No reset
note over UI: No changes applied
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/plugins/global-settings/typography/index.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugins/global-settings/typography/index.js (2)
src/plugins/global-settings/typography/utils.js (4)
getDevicePropertyKey(49-55)getDevicePropertyKey(49-55)cleanTypographyStyle(120-127)cleanTypographyStyle(120-127)src/plugins/global-settings/typography/editor-loader.js (1)
typographySettings(42-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP 6.6.2
Comment on lines
409
to
417
| if ( ! isEditingFontPair && currentFontPair ) { | ||
| // Clean style object to be consistent with changeStyles operation | ||
| const fontPairStyle = cleanTypographyStyle( currentFontPair.typography?.[ selector ] ) || {} | ||
| if ( ! isEqual( fontPairStyle, typographyStyle ) && ! Array.isArray( typographyStyle ) ) { | ||
| if ( ! isEqual( fontPairStyle, typographyStyleOnCurrentDevice ) && | ||
| ! Array.isArray( typographyStyleOnCurrentDevice ) | ||
| ) { | ||
| return true | ||
| } | ||
| return false |
There was a problem hiding this comment.
Comparison still trips reset in tablet/mobile
fontPairStyle retains the desktop/base keys (e.g. fontSize, fontSizeUnit), but typographyStyleOnCurrentDevice only keeps the current device keys (tabletFontSize, mobileFontSize, …). On tablet/mobile views this makes isEqual always fail—even when there are no device-specific overrides—so the Reset button still shows up. Please normalize the font pair snapshot to the current device before comparing.
- const fontPairStyle = cleanTypographyStyle( currentFontPair.typography?.[ selector ] ) || {}
- if ( ! isEqual( fontPairStyle, typographyStyleOnCurrentDevice ) &&
+ const fontPairStyle = cleanTypographyStyle( currentFontPair.typography?.[ selector ] ) || {}
+ const fontPairStyleOnCurrentDevice = cleanTypographyStyle( {
+ fontFamily: fontPairStyle?.fontFamily || '',
+ fontWeight: fontPairStyle?.fontWeight || '',
+ textTransform: fontPairStyle?.textTransform || '',
+ [ deviceFontSizeKey ]: fontPairStyle?.[ deviceFontSizeKey ] || '',
+ [ deviceFontSizeUnitKey ]: fontPairStyle?.[ deviceFontSizeUnitKey ] || '',
+ [ deviceLineHeightKey ]: fontPairStyle?.[ deviceLineHeightKey ] || '',
+ [ deviceLetterSpacingKey ]: fontPairStyle?.[ deviceLetterSpacingKey ] || '',
+ } ) || {}
+ if ( ! isEqual( fontPairStyleOnCurrentDevice, typographyStyleOnCurrentDevice ) &&📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ( ! isEditingFontPair && currentFontPair ) { | |
| // Clean style object to be consistent with changeStyles operation | |
| const fontPairStyle = cleanTypographyStyle( currentFontPair.typography?.[ selector ] ) || {} | |
| if ( ! isEqual( fontPairStyle, typographyStyle ) && ! Array.isArray( typographyStyle ) ) { | |
| if ( ! isEqual( fontPairStyle, typographyStyleOnCurrentDevice ) && | |
| ! Array.isArray( typographyStyleOnCurrentDevice ) | |
| ) { | |
| return true | |
| } | |
| return false | |
| if ( ! isEditingFontPair && currentFontPair ) { | |
| // Clean style object to be consistent with changeStyles operation | |
| const fontPairStyle = cleanTypographyStyle( currentFontPair.typography?.[ selector ] ) || {} | |
| const fontPairStyleOnCurrentDevice = cleanTypographyStyle( { | |
| fontFamily: fontPairStyle?.fontFamily || '', | |
| fontWeight: fontPairStyle?.fontWeight || '', | |
| textTransform: fontPairStyle?.textTransform || '', | |
| [ deviceFontSizeKey ]: fontPairStyle?.[ deviceFontSizeKey ] || '', | |
| [ deviceFontSizeUnitKey ]: fontPairStyle?.[ deviceFontSizeUnitKey ] || '', | |
| [ deviceLineHeightKey ]: fontPairStyle?.[ deviceLineHeightKey ] || '', | |
| [ deviceLetterSpacingKey ]: fontPairStyle?.[ deviceLetterSpacingKey ] || '', | |
| } ) || {} | |
| if ( ! isEqual( fontPairStyleOnCurrentDevice, typographyStyleOnCurrentDevice ) && | |
| ! Array.isArray( typographyStyleOnCurrentDevice ) | |
| ) { | |
| return true | |
| } | |
| return false |
🤖 Prompt for AI Agents
In src/plugins/global-settings/typography/index.js around lines 409-417, the
comparison fails because fontPairStyle contains desktop/base keys while
typographyStyleOnCurrentDevice contains only device-specific keys; normalize the
font pair snapshot to the current device before comparing by deriving a
device-specific object from currentFontPair.typography[selector] — map/translate
desktop/base properties to the current device equivalents (e.g., for device ===
'tablet' copy desktop fontSize/fontSizeUnit ->
tabletFontSize/tabletFontSizeUnit, same for mobile), then pass that normalized
device-specific object through cleanTypographyStyle and use isEqual against
typographyStyleOnCurrentDevice (keeping the existing Array.isArray guard).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…vice type
fixes #3550
Summary by CodeRabbit
Improvements
Bug Fixes