-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix resolver #61886
Fix resolver #61886
Changes from 9 commits
e8f7035
68d6d6f
6e63b1b
569e56a
35c4b9d
1cab6d2
192fcba
797a29c
2dc6242
0b505f5
0c8c03e
d658188
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ import { | |
euiPaletteForStatus, | ||
colorPalette, | ||
} from '@elastic/eui'; | ||
import styled from 'styled-components'; | ||
|
||
/** | ||
* Generating from `colorPalette` function: This could potentially | ||
|
@@ -396,17 +397,25 @@ const SymbolsAndShapes = memo(() => ( | |
)); | ||
|
||
/** | ||
* This <defs> element is used to define the reusable assets for the Resolver | ||
* It confers sevral advantages, including but not limited to: | ||
* 1) Freedom of form for creative assets (beyond box-model constraints) | ||
* 2) Separation of concerns between creative assets and more functional areas of the app | ||
* 3) <use> elements can be handled by compositor (faster) | ||
* This `<defs>` element is used to define the reusable assets for the Resolver | ||
* It confers several advantages, including but not limited to: | ||
* 1. Freedom of form for creative assets (beyond box-model constraints) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using markdown for IDEs that support it |
||
* 2. Separation of concerns between creative assets and more functional areas of the app | ||
* 3. `<use>` elements can be handled by compositor (faster) | ||
*/ | ||
export const SymbolDefinitions = memo(() => ( | ||
<svg> | ||
<defs> | ||
<PaintServers /> | ||
<SymbolsAndShapes /> | ||
</defs> | ||
</svg> | ||
)); | ||
export const SymbolDefinitions = styled( | ||
memo(({ className }: { className?: string }) => ( | ||
<svg className={className}> | ||
<defs> | ||
<PaintServers /> | ||
<SymbolsAndShapes /> | ||
</defs> | ||
</svg> | ||
)) | ||
)` | ||
position: absolute; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the most important change in this PR. Without this style change, the |
||
left: 100%; | ||
top: 100%; | ||
width: 0; | ||
height: 0; | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,15 +88,22 @@ export const Resolver = styled( | |
projectionMatrix={projectionMatrix} | ||
/> | ||
))} | ||
{Array.from(processNodePositions).map(([processEvent, position], index) => ( | ||
<ProcessEventDot | ||
key={index} | ||
position={position} | ||
projectionMatrix={projectionMatrix} | ||
event={processEvent} | ||
adjacentNodeMap={processToAdjacencyMap.get(processEvent)} | ||
/> | ||
))} | ||
{[...processNodePositions].map(([processEvent, position], index) => { | ||
const adjacentNodeMap = processToAdjacencyMap.get(processEvent); | ||
if (!adjacentNodeMap) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Asserting that this isn't falsey (it never should be) so that |
||
// This should never happen | ||
throw new Error('Issue calculating adjacency node map.'); | ||
} | ||
return ( | ||
<ProcessEventDot | ||
key={index} | ||
position={position} | ||
projectionMatrix={projectionMatrix} | ||
event={processEvent} | ||
adjacentNodeMap={adjacentNodeMap} | ||
/> | ||
); | ||
})} | ||
</StyledResolverContainer> | ||
)} | ||
<StyledPanel /> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,7 +82,7 @@ export const ProcessEventDot = styled( | |
/** | ||
* map of what nodes are "adjacent" to this one in "up, down, previous, next" directions | ||
*/ | ||
adjacentNodeMap?: AdjacentProcessMap; | ||
adjacentNodeMap: AdjacentProcessMap; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is no longer optional |
||
}) => { | ||
/** | ||
* Convert the position, which is in 'world' coordinates, to screen coordinates. | ||
|
@@ -91,7 +91,7 @@ export const ProcessEventDot = styled( | |
|
||
const [magFactorX] = projectionMatrix; | ||
|
||
const selfId = adjacentNodeMap?.self; | ||
const selfId = adjacentNodeMap.self; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No longer necessary to check if |
||
|
||
const nodeViewportStyle = useMemo( | ||
() => ({ | ||
|
@@ -117,27 +117,21 @@ export const ProcessEventDot = styled( | |
|
||
const labelYHeight = markerSize / 1.7647; | ||
|
||
const levelAttribute = adjacentNodeMap?.level | ||
? { | ||
'aria-level': adjacentNodeMap.level, | ||
} | ||
: {}; | ||
|
||
const flowToAttribute = adjacentNodeMap?.nextSibling | ||
? { | ||
'aria-flowto': adjacentNodeMap.nextSibling, | ||
} | ||
: {}; | ||
/** | ||
* An element that should be animated when the node is clicked. | ||
*/ | ||
const animationTarget: { current: SVGAnimationElement | null } = React.createRef(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed variable to |
||
const { cubeSymbol, labelFill, descriptionFill, descriptionText } = nodeAssets[ | ||
nodeType(event) | ||
]; | ||
const resolverNodeIdGenerator = useMemo(() => htmlIdGenerator('resolverNode'), []); | ||
|
||
const nodeType = getNodeType(event); | ||
const clickTargetRef: { current: SVGAnimationElement | null } = React.createRef(); | ||
const { cubeSymbol, labelFill, descriptionFill, descriptionText } = nodeAssets[nodeType]; | ||
const resolverNodeIdGenerator = htmlIdGenerator('resolverNode'); | ||
const [nodeId, labelId, descriptionId] = [ | ||
!!selfId ? resolverNodeIdGenerator(String(selfId)) : resolverNodeIdGenerator(), | ||
resolverNodeIdGenerator(), | ||
resolverNodeIdGenerator(), | ||
] as string[]; | ||
const nodeId = useMemo(() => resolverNodeIdGenerator(selfId), [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should only be generated once per instance of this component |
||
resolverNodeIdGenerator, | ||
selfId, | ||
]); | ||
const labelId = useMemo(() => resolverNodeIdGenerator(), [resolverNodeIdGenerator]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should only be generated once per instance of this component |
||
const descriptionId = useMemo(() => resolverNodeIdGenerator(), [resolverNodeIdGenerator]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should only be generated once per instance of this component |
||
|
||
const dispatch = useResolverDispatch(); | ||
|
||
|
@@ -154,14 +148,12 @@ export const ProcessEventDot = styled( | |
[dispatch, nodeId] | ||
); | ||
|
||
const handleClick = useCallback( | ||
(clickEvent: React.MouseEvent<SVGSVGElement, MouseEvent>) => { | ||
if (clickTargetRef.current !== null) { | ||
(clickTargetRef.current as any).beginElement(); | ||
} | ||
}, | ||
[clickTargetRef] | ||
); | ||
const handleClick = useCallback(() => { | ||
if (animationTarget.current !== null) { | ||
// Use undocumented `beginElement` API on SVG element | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explaining the use of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @oatkiller it's documented in SVG 1.1 ( Rec status since 16 Aug 2011) https://www.w3.org/TR/SVG11/animate.html#__smil__ElementTimeControl__beginElement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add that link to the comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
(animationTarget.current as any).beginElement(); | ||
} | ||
}, [animationTarget]); | ||
|
||
return ( | ||
<EuiKeyboardAccessible> | ||
|
@@ -171,8 +163,8 @@ export const ProcessEventDot = styled( | |
viewBox="-15 -15 90 30" | ||
preserveAspectRatio="xMidYMid meet" | ||
role="treeitem" | ||
{...levelAttribute} | ||
{...flowToAttribute} | ||
aria-level={adjacentNodeMap.level} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assign these directly, as the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @oatkiller that's OK for level, but aria-flowto can't be empty/null so I think that one still has to be spread There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typescript caught this as well. ty |
||
aria-flowto={adjacentNodeMap.nextSibling} | ||
aria-labelledby={labelId} | ||
aria-describedby={descriptionId} | ||
aria-haspopup={'true'} | ||
|
@@ -202,7 +194,7 @@ export const ProcessEventDot = styled( | |
begin="click" | ||
repeatCount="1" | ||
className="squish" | ||
ref={clickTargetRef} | ||
ref={animationTarget} | ||
/> | ||
</use> | ||
<use | ||
|
@@ -273,7 +265,7 @@ const processTypeToCube: Record<ResolverProcessType, keyof typeof nodeAssets> = | |
unknownEvent: 'runningProcessCube', | ||
}; | ||
|
||
function getNodeType(processEvent: ResolverEvent): keyof typeof nodeAssets { | ||
function nodeType(processEvent: ResolverEvent): keyof typeof nodeAssets { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removing |
||
const processType = processModel.eventType(processEvent); | ||
|
||
if (processType in processTypeToCube) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oatkiller this might actually not be true... e.g. when someone switches the active descendant (with the Panel), the tree node doesn't have/get focus until it's returned to its parent composite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean. But if you have a better suggestion for this, please let me know. Resolver already has a concept of 'focusing' on a process. It happens when you click the icon in the panel and the graph moves to center the process. This action should probably be combined with that one. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the
role
description for tree,focus
andactivedescendant
are independent concepts. When the user clicks the icon in the panel, the icon should still have input focus, but the Resolver'saria-activedescendant
changes to the node's ID and when focus returns to the tree/(Resolver) you assign focus back to theactivedescendant
node.With this is mind, my suggestion might be to change this interface name to something like
userSelectedResolverNode
but we're going to be taking a swipe at this whole active/focus thing soon so feel free to let it ride for now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I understand. Basically you're saying that the 'selected' (our definition) resolver node and the focused (DOM's definition) element aren't necessarily (and shouldn't be) the same?