Skip to content

Conversation

@steelsojka
Copy link
Collaborator

@steelsojka steelsojka commented Oct 31, 2025

  • Adds scrolling while dragging a component toward the viewport boundaries.
  • Fixes an issue while the mouse is leaving a component that is a child of another component. The parent component would not get highlighted in this scenario.
  • Simplified connection async logic

@steelsojka steelsojka requested a review from a team as a code owner October 31, 2025 14:59
@steelsojka steelsojka requested review from mjuraschik and removed request for a team October 31, 2025 15:01
() => ({
isDesignMode: isDesignModeActive(),
isPreviewMode: isPreviewModeActive(),
isDesignMode: mode === 'design' || isDesignModeActive(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This allows for us to set a mode programmatically when consumed. Useful if we want an alternative to the url parameter, for example, in storybook we can't control the url parameters. In Odyssey we won't provide a mode so this will default to the behavior it had before.

ComponentPropertiesChanged: Domain.ComponentPropertiesChangedEvent;
MediaChangedEvent: Domain.MediaChangedEvent;
ClientWindowBoundsHoverOver: Domain.ClientWindowBoundsHoverOverEvent;
ClientWindowBoundsHoverOut: Domain.ClientWindowBoundsHoverOutEvent;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did we find that we just don't need these?

}): (query: {
x: number;
y: number;
filter?: (entry: NodeToTargetMapEntry) => boolean;
Copy link
Collaborator

@rfding rfding Oct 31, 2025

Choose a reason for hiding this comment

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

What does filter do? It's an optional parameter that users can pass into the function that gets returned by useComponentDiscovery? Where do we expect to use this?

const node = nodeStack[i];
const entry = nodeToTargetMap.get(node);

// We need a region id and direction for this to be a target.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we actually filtering on region id and direction here? Would they have to pass in a filter function that does that?

Copy link
Collaborator

@rfding rfding left a comment

Choose a reason for hiding this comment

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

Are we able to add any unit tests for this type of functionality or is this more of an integration test thing?

const SCROLL_INTERVAL_IN_MS = 1000 / 60; // 60fps
// The multiplier applied to the scroll factor.
// The scroll factor is a value between 0 and 1 that determines how much to scroll.
// This value will be the maximum amount of pixels that will be scrolled in a singal frame.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// This value will be the maximum amount of pixels that will be scrolled in a singal frame.
// This value will be the maximum amount of pixels that will be scrolled in a single frame.

for (let i = 0; i < stack.length; i += 1) {
const entry = stack[i];

// We need a region id and direction for this to be a target.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just pass a filter function into discoverComponents instead of having this outer if check here?

isDragging: true,
currentDropTarget: null,
pendingTargetCommit: false,
scrollDirection: computeScrollDirection(scrollFactorRef.current),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this just be 0 all the time?

Suggested change
scrollDirection: computeScrollDirection(scrollFactorRef.current),
scrollDirection: 0,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants