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.

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

Improve the design of workflow nodes #9810

Merged
merged 6 commits into from
Jan 23, 2025
Merged

Conversation

Devessier
Copy link
Contributor

@Devessier Devessier commented Jan 23, 2025

  • Go over every node in the workflows and fix the styles to conform to Figma
  • Create stories for every node type

Comment on lines 10 to 18
const NODE_HANDLE_WIDTH_PX = 4;
const NODE_HANDLE_HEIGHT_PX = NODE_HANDLE_WIDTH_PX;
export const NODE_ICON_WIDTH = Number(
THEME_COMMON.spacing(6).replace('px', ''),
);
export const NODE_ICON_LEFT_MARGIN = Number(
THEME_COMMON.spacing(2).replace('px', ''),
);
export const NODE_BORDER_WIDTH = 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really know where to put these constants.

Copy link
Contributor

Choose a reason for hiding this comment

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

why not in a separated file in workflow-diagram/constants/

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR enhances workflow node design with improved styling consistency and component organization.

  • Standardized icon sizes across nodes using theme.icon.size.md instead of hardcoded values in /packages/twenty-front/src/modules/workflow/workflow-diagram/components/WorkflowDiagramStepNodeBase.tsx
  • Added constants for node dimensions and spacing (NODE_HANDLE_WIDTH_PX, NODE_ICON_WIDTH) for better maintainability
  • Improved visual hierarchy with refined font sizes (9px for node type, 13px for label) and box shadow styling
  • Created new WorkflowDiagramStepNodeEditableContent component for better separation of concerns with delete functionality
  • Added comprehensive Storybook stories showcasing different workflow node states and configurations

9 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +14 to +13
transform: translateX(-50%);
position: relative;
left: ${NODE_ICON_WIDTH + NODE_ICON_LEFT_MARGIN + NODE_BORDER_WIDTH}px;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Using both transform: translateX(-50%) and explicit left positioning could lead to unexpected positioning behavior. Consider using one or the other.

Comment on lines +94 to 91
const StyledTargetHandle = styled(StyledSourceHandle)`
left: ${NODE_ICON_WIDTH + NODE_ICON_LEFT_MARGIN + NODE_BORDER_WIDTH}px;
visibility: hidden;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: StyledTargetHandle duplicates the left position from StyledSourceHandle unnecessarily since it inherits from it

Comment on lines +19 to +23
<FloatingIconButton
size="medium"
Icon={IconTrash}
onClick={onDelete}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Missing aria-label for accessibility on delete button

<FloatingIconButton
size="medium"
Icon={IconTrash}
onClick={onDelete}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider adding stopPropagation() to prevent node selection from being triggered when deleting

import { Meta, StoryObj } from '@storybook/react';
import { ComponentDecorator } from 'twenty-ui';

import { ReactFlowProvider } from '@xyflow/react';
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: missing import for @xyflow/react/dist/style.css which is required for ReactFlow to render correctly

Comment on lines 28 to 33
ComponentDecorator,
(Story) => (
<ReactFlowProvider>
<Story />
</ReactFlowProvider>
),
Copy link
Contributor

Choose a reason for hiding this comment

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

style: duplicate ReactFlowProvider decorator from Default story could be extracted to meta.decorators to avoid repetition

@@ -21,7 +31,7 @@ const StyledStepNodeType = styled.div`
${({ theme }) => theme.border.radius.sm} 0 0;

color: ${({ theme }) => theme.font.color.light};
font-size: ${({ theme }) => theme.font.size.md};
font-size: 9px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thomas wants these pixel-perfect values. We can replace these hard-coded values with tokens from the design system later.

@@ -61,7 +70,7 @@ const StyledStepNodeInnerContainer = styled.div<{ variant?: Variant }>`
const StyledStepNodeLabel = styled.div<{ variant?: Variant }>`
align-items: center;
display: flex;
font-size: ${({ theme }) => theme.font.size.lg};
font-size: 13px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idem about pixel-perfect values.

Comment on lines 10 to 18
const NODE_HANDLE_WIDTH_PX = 4;
const NODE_HANDLE_HEIGHT_PX = NODE_HANDLE_WIDTH_PX;
export const NODE_ICON_WIDTH = Number(
THEME_COMMON.spacing(6).replace('px', ''),
);
export const NODE_ICON_LEFT_MARGIN = Number(
THEME_COMMON.spacing(2).replace('px', ''),
);
export const NODE_BORDER_WIDTH = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

why not in a separated file in workflow-diagram/constants/

@Devessier Devessier enabled auto-merge (squash) January 23, 2025 15:09
@Devessier Devessier merged commit bbb0c9a into main Jan 23, 2025
47 checks passed
@Devessier Devessier deleted the workflow-nodes-design-fix branch January 23, 2025 15:12
Copy link

Fails
🚫

node failed.

Log

�[31mError: �[39m SyntaxError: Unexpected token C in JSON at position 0
    at JSON.parse (<anonymous>)
�[90m    at parseJSONFromBytes (node:internal/deps/undici/undici:5584:19)�[39m
�[90m    at successSteps (node:internal/deps/undici/undici:5555:27)�[39m
�[90m    at fullyReadBody (node:internal/deps/undici/undici:1665:9)�[39m
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m
�[90m    at async specConsumeBody (node:internal/deps/undici/undici:5564:7)�[39m
danger-results://tmp/danger-results-0ac83204.json

Generated by 🚫 dangerJS against 002d540

DeepaPrasanna pushed a commit to DeepaPrasanna/twenty that referenced this pull request Jan 27, 2025
- Go over every node in the workflows and fix the styles to conform to
Figma
- Create stories for every node type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants