Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Arrow for Diagram Component #6863

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
+453 −1
Open

feat: Arrow for Diagram Component #6863

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
9f64ad0
feat: Arrow for Diagram Component
g4rry420 Jul 5, 2023
fd2b6c5
chore: Changed the API for arrow to endpoint prop with extensibility …
g4rry420 Jul 8, 2023
3c624d7
Added a snapshot test for endpoint - arrow prop and resolves the ts t…
g4rry420 Jul 8, 2023
6e30aca
lint: Added comma in propTypes
g4rry420 Jul 8, 2023
f871fda
Merge branch 'master' into issue_5882
g4rry420 Jul 8, 2023
85f1c68
Merge branch 'master' into issue_5882
g4rry420 Jul 21, 2023
4337791
changed the id name for the marker, fixed the duplicate issue with ar…
g4rry420 Jul 22, 2023
2bd7bee
fix: arrow appearing in another line if they have same color
g4rry420 Jul 22, 2023
0bd64e0
fix: linting issue
g4rry420 Jul 22, 2023
8974d44
Merge branch 'master' into issue_5882
g4rry420 Aug 23, 2023
5d4129d
chore: added fromTarget and toTarget in id name of the marker
g4rry420 Oct 9, 2023
dfbf756
chore: changed the id of start and end open arrow
g4rry420 Oct 25, 2023
f86aa56
chore: updated snapshot"
g4rry420 Oct 27, 2023
54ffa1d
Merge branch 'master' into issue_5882
g4rry420 Nov 3, 2023
50fc9da
chore: Added colorName in the id of the arrow as it was causing error…
g4rry420 Nov 4, 2023
717a177
Merge branch 'master' into issue_5882
g4rry420 Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
108 changes: 94 additions & 14 deletions src/js/components/Diagram/Diagram.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React, {
Fragment,
forwardRef,
useCallback,
useContext,
Expand Down Expand Up @@ -66,6 +67,19 @@ const findTarget = (target) => {
return target;
};

const openArrow = (color, index, name = 'openArrowEnd', orient = 'auto') => (
<marker
id={`${name}-${index}`}
markerWidth="7"
markerHeight="9"
refX="2"
refY="6"
orient={orient}
>
<path d="M1,4 L3,6 L1,8" stroke={color} fill="none" />
</marker>
);

const Diagram = forwardRef(({ connections, ...rest }, ref) => {
const theme = useContext(ThemeContext) || defaultProps.theme;
const [dimensions, setDimensions] = useState({ width: 0, height: 0 });
Expand Down Expand Up @@ -117,7 +131,7 @@ const Diagram = forwardRef(({ connections, ...rest }, ref) => {
const placeConnections = useCallback(() => {
const containerRect = svgRef.current.getBoundingClientRect();
const updatedConnectionPoints = connections.map(
({ anchor, fromTarget, toTarget }) => {
({ anchor, fromTarget, toTarget, endpoint }) => {
let points;
const fromElement = findTarget(fromTarget);
const toElement = findTarget(toTarget);
Expand Down Expand Up @@ -145,16 +159,52 @@ const Diagram = forwardRef(({ connections, ...rest }, ref) => {
toPoint[0] += toRect.width / 2;
if (fromRect.top < toRect.top) {
fromPoint[1] += fromRect.height;

if (typeof endpoint === 'object' && endpoint?.from) {
fromPoint[1] += 10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How was 10 determined? It might be helpful to add a code comment for future.

Copy link
Contributor Author

@g4rry420 g4rry420 Jul 21, 2023 •
edited

Choose a reason for hiding this comment

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

I haven't used a definitive approach to calculate the 10. I just check the Animated story and 10 seems good which isn't closely aligned with the start and end of the main diamond diagram

} else if (typeof endpoint === 'object' && endpoint?.to) {
toPoint[1] -= 10;
} else if (typeof endpoint === 'string') {
fromPoint[1] += 10;
toPoint[1] -= 10;
}
} else {
toPoint[1] += toRect.height;

if (typeof endpoint === 'object' && endpoint?.from) {
fromPoint[1] -= 10;
} else if (typeof endpoint === 'object' && endpoint?.to) {
toPoint[1] += 10;
} else if (typeof endpoint === 'string') {
fromPoint[1] -= 10;
toPoint[1] += 10;
}
}
} else if (anchor === 'horizontal') {
fromPoint[1] += fromRect.height / 2;
toPoint[1] += toRect.height / 2;
if (fromRect.left < toRect.left) {
fromPoint[0] += fromRect.width;

if (typeof endpoint === 'object' && endpoint?.from) {
fromPoint[0] += 10;
} else if (typeof endpoint === 'object' && endpoint?.to) {
toPoint[0] -= 10;
} else if (typeof endpoint === 'string') {
fromPoint[0] += 10;
toPoint[0] -= 10;
}
} else {
toPoint[0] += toRect.width;

if (typeof endpoint === 'object' && endpoint?.from) {
fromPoint[0] -= 10;
} else if (typeof endpoint === 'object' && endpoint?.to) {
toPoint[0] += 10;
} else if (typeof endpoint === 'string') {
fromPoint[0] -= 10;
toPoint[0] += 10;
}
}
} else {
// center
Expand Down Expand Up @@ -190,6 +240,7 @@ const Diagram = forwardRef(({ connections, ...rest }, ref) => {
round,
thickness,
type,
endpoint,
...connectionRest
},
index,
Expand Down Expand Up @@ -222,20 +273,49 @@ const Diagram = forwardRef(({ connections, ...rest }, ref) => {
colorName = colors[index % colors.length];
}

let arrowMarker = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a smaller comment but to be more flexible for future, should we name this variable endpoint? If needed we can alias the prop as endpointProp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since, endpoint is already a prop we are adding in Diagram component, naming the arrowMarker variable to endpointProp will create a confusion as it's expected type to be 'arrow' | { from?: 'arrow'; to?: 'arrow' }. How about if we name this variable as endpointMarker ?
The reason, I have choosed this name - endpointMarker variable is because we are storing the marker tag with the openArrow in it and in future I believe for other endpoints we will be using marker for it. Please, let me know your thoughts


if (typeof endpoint === 'object' && endpoint?.from === 'arrow') {
arrowMarker = openArrow(
normalizeColor(colorName, theme),
index,
'openArrowStart',
Copy link
Collaborator

Choose a reason for hiding this comment

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

also to be more flexible for future, should we name this endpointStart since it might not always be an arrow?

'auto-start-reverse',
);
} else if (typeof endpoint === 'object' && endpoint?.to === 'arrow') {
arrowMarker = openArrow(normalizeColor(colorName, theme), index);
} else if (typeof endpoint === 'string' && endpoint === 'arrow') {
arrowMarker = (
<>
{openArrow(
normalizeColor(colorName, theme),
index,
'openArrowStart',
'auto-start-reverse',
)}
{openArrow(normalizeColor(colorName, theme), index)}
</>
);
}

path = (
<path
// eslint-disable-next-line react/no-array-index-key
key={index}
// eslint-disable-next-line react/no-unknown-property
animation={animation}
{...cleanedRest}
stroke={normalizeColor(colorName, theme)}
strokeWidth={strokeWidth}
strokeLinecap={round ? 'round' : 'butt'}
strokeLinejoin={round ? 'round' : 'miter'}
fill="none"
d={d}
/>
// eslint-disable-next-line react/no-array-index-key
<Fragment key={index}>
<path
// eslint-disable-next-line react/no-unknown-property
animation={animation}
{...cleanedRest}
stroke={normalizeColor(colorName, theme)}
strokeWidth={strokeWidth}
strokeLinecap={round ? 'round' : 'butt'}
strokeLinejoin={round ? 'round' : 'miter'}
fill="none"
d={d}
markerStart={`url("#openArrowStart-${index}")`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any downside to having the markerStart/markerEnd here if the path doesn't have an arrow?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, if multiple diagrams are rendered on a page, will this index id approach work still?

Copy link
Contributor Author

@g4rry420 g4rry420 Jul 21, 2023 •
edited

Choose a reason for hiding this comment

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

Yes, there is, I will fix that.
Yes, currently, if multiple diagrams are rendered on page, the index approach will not work. I am working on new approach.

markerEnd={`url("#openArrowEnd-${index}")`}
/>
{endpoint && arrowMarker && <defs>{arrowMarker}</defs>}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we wrap in defs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main use case for defs was to holds a set of reusable definitions for the SVG markers. But, it isn't serving it's purpose right now, I will push some change in which we will using arrow based on it's open, close and color.

</Fragment>
);
}
return path;
Expand Down
30 changes: 30 additions & 0 deletions src/js/components/Diagram/__tests__/Diagram-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,34 @@ describe('Diagram', () => {
);
expect(container.firstChild).toMatchSnapshot();
});

test('endpoint - arrow', () => {
const { container } = render(
<Context>
<Diagram
connections={[
{
fromTarget: '1',
toTarget: '2',
endpoint: 'arrow',
anchor: 'center',
},
{
fromTarget: '1',
toTarget: '2',
endpoint: { from: 'arrow' },
anchor: 'horizontal',
},
{
fromTarget: '1',
toTarget: '2',
endpoint: { to: 'arrow' },
anchor: 'vertical',
},
]}
/>
</Context>,
);
expect(container.firstChild).toMatchSnapshot();
});
});