Reset for colours of BG colour on hover state #6027
Conversation
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as BackgroundLayersControl / Color UI
participant CL as ColorLayer.getResetValue
participant BlockAttrs as Block Attributes (IB)
participant Store as Breakpoint Data (non-IB)
User->>UI: Click "Reset" on a color layer (maybe in hover)
UI->>CL: onReset() -> getResetValue(isLayer, isHover, breakpoint, prevBreakpoint, blockAttributes)
alt isLayer (IB) path
CL->>BlockAttrs: Read target layer attributes (prefer hover value)
BlockAttrs-->>CL: Resolved attribute(s)
else non-IB path
alt prevBreakpoint exists
CL->>Store: Query prevBreakpoint data with isHover=false
Store-->>CL: Prev breakpoint color
else
CL->>Store: Query current breakpoint data with isHover=false
Store-->>CL: Current non-hover color
end
end
CL-->>UI: return reset value (includes isReset: true)
UI->>UI: Emit onChangeColor(payload including isReset)
UI-->>User: UI updates color control
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
dosubot
bot
added
the
size:XS
Olekrut
left a comment
Olekrut
left a comment
There was a problem hiding this comment.
It works for hover but not for IB
test hoverand IB for BG colour.css
…nto Reset-for-colours-of-BG-colour-on-hover-state
…nto Reset-for-colours-of-BG-colour-on-hover-state
dosubot
bot
added
size:L
![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 (4)
src/components/background-control/backgroundLayersControl.js(6 hunks)src/components/background-control/blockBackgroundControl.js(2 hunks)src/components/background-control/colorLayer.js(6 hunks)src/components/relation-control/index.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/background-control/colorLayer.js (2)
src/components/color-control/index.js (1)
showPalette(133-133)src/extensions/style-cards/getSCVariablesObject.js (1)
paletteOpacity(23-23)
src/components/background-control/backgroundLayersControl.js (1)
src/components/background-control/utils.js (4)
layer(81-81)layer(123-123)handleOnChangeLayer(148-160)handleOnChangeLayer(148-160)
⏰ 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). (2)
- GitHub Check: e2e-js
- GitHub Check: unit-js
🔇 Additional comments (7)
src/components/relation-control/index.js (1)
223-223: Pass isHover flag—ensure downstream consumption
The newisHover: item.effects?.hoverStatus || falseprop correctly surfaces hover state. Verify that the settings components receiving this prop explicitly destructure and useisHoverin their reset handlers to restore the normal-state background color.src/components/background-control/blockBackgroundControl.js (1)
40-40: LGTM!The propagation of
blockAttributesthrough toBackgroundLayersControlis correct and necessary for enabling IB (inline block) reset functionality downstream.Also applies to: 88-88
src/components/background-control/backgroundLayersControl.js (3)
73-82: LGTM!The helper function correctly locates the matching background layer from the target block's attributes by order and type. Returning
nullfor non-IB cases or when no match is found is handled safely downstream.
100-106: LGTM!The destructuring pattern correctly strips
isResetbefore passing the change object tohandleOnChangeLayer(which expects layer attribute updates only), then reattaches it to preserve the reset flag for the final change emission.
117-117: LGTM!Passing
blockAttributes={getBlockLayer()}toColorLayerprovides the necessary context for IB reset resolution. Only color layers receive this prop, which is appropriate since the reset behavior is specific to color layers.src/components/background-control/colorLayer.js (2)
152-172: LGTM!The non-IB reset logic correctly resolves hover mode reset to the current breakpoint's normal state value (lines 152-158), and non-hover mode reset to the previous breakpoint or current breakpoint's normal state (lines 159-171). This matches the expected behavior described in the issue.
225-253: LGTM!The
onResetfunction correctly propagatesisReset: truein both palette-enabled (lines 225-233) and palette-disabled (lines 234-253) code paths, ensuring the reset flag is properly threaded through the change emission.
| if (isHover) { | ||
| const blockHoverValue = getLastBreakpointAttribute({ | ||
| target, | ||
| breakpoint, | ||
| attributes: blockAttributes, | ||
| isHover: true, | ||
| }); | ||
|
|
||
| if (blockHoverValue !== undefined) { | ||
| return blockHoverValue; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify hover reset behavior matches issue requirements.
The IB hover reset logic first attempts to retrieve the target block's hover value (lines 111-122), then falls back to the normal value (lines 124-134). However, the issue description states: "Clicking the reset button in hover state should revert the hover colour to the colour selected for the normal state."
This suggests the reset should always revert to the normal state color, not the hover state color. The non-IB logic (lines 152-158) correctly implements this by getting isHover=false when in hover mode.
Please verify the intended behavior:
- Should IB hover reset always use the target block's normal state color (matching the issue description)?
- Or should it prefer the target block's hover color if defined (current implementation)?
If the former, apply this diff to skip the hover value check:
if (isIB) {
// For IB reset, we want to reset to the target block's actual background color
// NOT the IB override values - we're clearing the IB override!
// First, get the value from the target block's actual attributes
- // If we're in hover mode, check for the block's hover value first
if (blockAttributes) {
- // Try to get the hover value if we're in hover mode
- if (isHover) {
- const blockHoverValue = getLastBreakpointAttribute({
- target,
- breakpoint,
- attributes: blockAttributes,
- isHover: true,
- });
-
- if (blockHoverValue !== undefined) {
- return blockHoverValue;
- }
- }
-
- // If not in hover mode, or no hover value exists, get non-hover value
+ // Get non-hover value (normal state)
const blockValue = getLastBreakpointAttribute({
target,
breakpoint,
attributes: blockAttributes,
isHover: false,
});🏁 Script executed:
#!/bin/bash
set -e
FILE="src/components/background-control/colorLayer.js"
echo "Showing lines 80–180 of $FILE"
sed -n '80,180p' "$FILE"Length of output: 2811
Remove hover-first check in IB reset logic
The IB reset path must always revert to the normal-state color. Remove the hover-value lookup (lines 111–122) so getResetValue only fetches isHover: false.
🤖 Prompt for AI Agents
In src/components/background-control/colorLayer.js around lines 111 to 122, the
IB reset logic currently checks for and returns a hover-state value; remove that
hover-value lookup so the reset path always reverts to the normal state. Delete
the if (isHover) { ... getLastBreakpointAttribute(... isHover: true) ... } block
and ensure getResetValue only calls getLastBreakpointAttribute with isHover:
false (or omits isHover/sets to false) so the reset always fetches the non-hover
color.
Fixes #5783
Summary by CodeRabbit
New Features
Bug Fixes