feat(module:splitter): add splitter component #8987

Laffery · 2025-01-21T03:44:26Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/NG-ZORRO/ng-zorro-antd/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Application (the showcase website) / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: #8940

What is the new behavior?

Add splitter component, see https://ant.design/components/splitter

Does this PR introduce a breaking change?

Yes
No

Other information

zorro-bot · 2025-01-21T03:44:46Z

This preview will be available after the AzureCI is passed.

codecov · 2025-01-21T03:50:04Z

Codecov Report

Attention: Patch coverage is 54.95050% with 91 lines in your changes missing coverage. Please review.

Project coverage is 91.56%. Comparing base (f0e9d7f) to head (0341f5e).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
components/splitter/splitter.component.ts	48.12%	74 Missing and 9 partials ⚠️
components/splitter/splitter-bar.component.ts	72.72%	6 Missing ⚠️
components/splitter/utils.ts	77.77%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8987      +/-   ##
==========================================
- Coverage   91.94%   91.56%   -0.39%     
==========================================
  Files         559      564       +5     
  Lines       19773    19975     +202     
  Branches     3050     3089      +39     
==========================================
+ Hits        18181    18290     +109     
- Misses       1267     1349      +82     
- Partials      325      336      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lppedd · 2025-03-16T12:42:16Z

components/splitter/splitter.component.ts

+})
+export class NzSplitterComponent {
+  /** ------------------- Props ------------------- */
+  nzLayout = input<NzSplitterLayout>('horizontal');


Now that inputs are Signals, does it make to declare everything as readonly as you did for outputs?

You're right, although readonly isn't required to inputs, we can still use it to make sure type safe during runtime!

Indeed. I also like a consistent standard - so readonly by default (where applicable) is how I'd do it.

lppedd · 2025-03-16T14:18:00Z

components/splitter/splitter-bar.component.ts

+  lazy = input(false);
+  constrainedOffset = input<number>();
+
+  readonly previewTransform = computed(() => {


Another opinion: I see that you used protected for resizeStartEvent, but not for properties (those signals) used in templates. This is another candiate for a project-wide standard: should all template-only props/funs be marked as protected, or should all be public?

We do not expect user to use those internally-used-only props/funs, which are unstable and may be changed/removed in some refactoring PRs.

We only promise that public props/funs declared at the documentation website are compatible in a minor version. So I think they shall be protected.
But sometimes it's not convenient to access them in unit test cases.

TBH, there is currrently no consistant standard in this project.
We cannot get the acknownledge of how many internal apis are being used by user, what we can do as a popular component library is to keep public apis in stable and avoid exposing too many unstable internal apis.

TBH, there is no consistant standard in this project.

Yeah that's what I saw by reading the code.

I would go the route of not bothering with specifying protected, and instead keep everything public because:

sometimes users need access to unstable / internal declarations when NG-ZORRO doesn't behave as expected

you could alternatively mark unstable / internal declarations with TSDoc's @internal

/** @internal */ readonly previewTransform = computed(() => { ... })

to signify to consumers this is not something they should normally use.
Additionaly, @internal plays nicely with stripInternal in case in the future you want to get rid of them in the outputted types.

Btw, up to you. I was mostly trying to understand if there is a project/team standard, especially now that components are being refactored or created to support signals.

Laffery · 2025-03-16T15:55:27Z

@lppedd I'd made changes as your suggestions

Laffery · 2025-03-16T16:06:14Z

This pr is nearly completed, all the use cases of antd are implemented.

There are Todos as follows till now:

Restore the previous size when expanding a collapsed panel
Observe to resize event of the container
RTL support
Unit test cases

I will add them next week, and I think they do not have a impact on your review of the code, so could you please take a review on this PR if you have time. @Nicoss54 @HyperLife1119 @OriginRing @lppedd

lppedd

Left some minor comments / nitpicks.

components/splitter/splitter.component.ts

lppedd · 2025-03-16T16:16:11Z

components/splitter/splitter.component.ts

+  readonly destroy$ = inject(NzDestroyService);
+  readonly elementRef = inject<ElementRef<HTMLElement>>(ElementRef);


inject-ed services or elements are best kept private imo.
I'd also keep a consistent structure:

private properties

inputs

outputs

component state (props/functions)

I prefer the following order

inputs

outputs

private properties

component state (props/functions)

Puting inputs/outputs at the very top makes it clear to the person viewing the code how the component is used, just like a instruction manual

Consistency over order, so that looks good to me.

components/splitter/splitter.component.ts

components/splitter/typings.ts

components/splitter/doc/index.en-US.md

components/splitter/splitter-bar.component.ts

Nicoss54 · 2025-03-16T18:23:05Z

@Laffery @HyperLife1119 does it have a guideline for refactoring the components to support signal ? It will be a great pleasure to help you on this huge reworking.

Take a look and give my review tomorrow if it’s good for you ?

HyperLife1119 · 2025-03-17T02:23:50Z

@Nicoss54 If there is no special situation, existing components will not be migrated to signals.

Laffery · 2025-03-17T02:48:24Z

@Nicoss54 If there is no special situation, existing components will not be migrated to signals.

Agree. That would be a huge work and I don't think the payoff would be great, and there could be unexpected problems introduced.
I think we can leave it as is for now, unless there are bottlenecks in feature implementation or performance.
For new components, we 100% recommend using signals.

Laffery · 2025-03-17T02:51:34Z

I think we can leave it as is for now, unless there are bottlenecks in feature implementation or performance. For new components, we 100% recommend using signals.

By the way, material still remains in the original implementation as well

lppedd · 2025-03-17T12:15:12Z

Converting to signals during a component's fix or feature addition might be useful to expose unexpected property writes from inside or outside the component itself. But apart from that I agree and I don't see the urgency (also considering signals are not strictly necessary for zoneless).

HyperLife1119 · 2025-03-20T10:17:52Z

components/splitter/splitter-bar.component.ts

+      (mousedown)="resizeStartEvent($event)"
+      (touchstart)="resizeStartEvent($event)"


Maybe we can use Pointer Event, which supports both mouse and touch.

I tried it and the drag doesn't work very well with a touch screen.
Currently antd uses mousedown and touchstart as well, I think we can keep aligning with them.

components/splitter/splitter.component.ts

Laffery added ⭐ New Component PR: draft PR: target-minor Component: Splitter labels Jan 21, 2025

Laffery self-assigned this Jan 21, 2025

Laffery added this to the v19.1 milestone Jan 21, 2025

Laffery changed the title ~~feat(module:splitter): add splitter component~~ WIP feat(module:splitter): add splitter component Jan 21, 2025

Laffery force-pushed the feat/splitter branch 2 times, most recently from 6f96ac0 to 7da5053 Compare January 21, 2025 07:03

Laffery modified the milestones: v19.1, v19.2 Feb 19, 2025

Laffery force-pushed the feat/splitter branch from 7da5053 to 4021546 Compare March 13, 2025 23:16

Laffery requested a review from CK110 as a code owner March 14, 2025 15:46

Laffery force-pushed the feat/splitter branch from ced230c to bec0adb Compare March 14, 2025 15:57

Laffery added 3 commits March 15, 2025 20:14

feat(module:splitter): init showcases

992132a

feat(module:splitter): basic usage

8273e04

feat(module:splitter): add split bar directive

c203e4d

Laffery force-pushed the feat/splitter branch from bec0adb to b322854 Compare March 15, 2025 12:16

feat(module:splitter): handle resizing

4cc3d29

Laffery force-pushed the feat/splitter branch from 4142df6 to 4cc3d29 Compare March 15, 2025 16:20

Laffery added 3 commits March 16, 2025 13:29

feat(module:splitter): opt styles

0910540

feat(module:splitter): control mode

ad8c439

feat(module:splitter): lazy mode

d3fa455

lppedd reviewed Mar 16, 2025

View reviewed changes

feat(module:splitter): add collapse bar

2354a31

lppedd reviewed Mar 16, 2025

View reviewed changes

feat(module:splitter): add collapse

6d670ab

Laffery requested review from lppedd, Nicoss54, HyperLife1119 and OriginRing March 16, 2025 16:06

Laffery added PR: unreviewed and removed PR: draft labels Mar 16, 2025

Laffery changed the title ~~WIP feat(module:splitter): add splitter component~~ feat(module:splitter): add splitter component Mar 16, 2025

lppedd reviewed Mar 16, 2025

View reviewed changes

feat(module:splitter): add comments

8df8c65

Laffery added 2 commits March 19, 2025 21:45

feat(module:splitter): observe to container size change

e0f9019

feat(module:splitter): improve collapsible logic

dd8711d

HyperLife1119 requested changes Mar 20, 2025

View reviewed changes

feat(module:splitter): add placeholder of tests

0341f5e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(module:splitter): add splitter component #8987

feat(module:splitter): add splitter component #8987

Laffery commented Jan 21, 2025

zorro-bot bot commented Jan 21, 2025

codecov bot commented Jan 21, 2025 •

edited

Loading

lppedd Mar 16, 2025

Laffery Mar 16, 2025

lppedd Mar 16, 2025

lppedd Mar 16, 2025

Laffery Mar 16, 2025 •

edited

Loading

lppedd Mar 16, 2025

lppedd Mar 16, 2025

Laffery commented Mar 16, 2025

Laffery commented Mar 16, 2025 •

edited

Loading

lppedd left a comment

lppedd Mar 16, 2025

Laffery Mar 17, 2025

lppedd Mar 17, 2025

Nicoss54 commented Mar 16, 2025

HyperLife1119 commented Mar 17, 2025

Laffery commented Mar 17, 2025

Laffery commented Mar 17, 2025

lppedd commented Mar 17, 2025

HyperLife1119 Mar 20, 2025

Laffery Mar 21, 2025

		readonly destroy$ = inject(NzDestroyService);
		readonly elementRef = inject<ElementRef<HTMLElement>>(ElementRef);

		(mousedown)="resizeStartEvent($event)"
		(touchstart)="resizeStartEvent($event)"

feat(module:splitter): add splitter component #8987

Are you sure you want to change the base?

feat(module:splitter): add splitter component #8987

Conversation

Laffery commented Jan 21, 2025

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

zorro-bot bot commented Jan 21, 2025

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laffery Mar 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laffery commented Mar 16, 2025

Laffery commented Mar 16, 2025 • edited Loading

lppedd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nicoss54 commented Mar 16, 2025

HyperLife1119 commented Mar 17, 2025

Laffery commented Mar 17, 2025

Laffery commented Mar 17, 2025

lppedd commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 21, 2025 •

edited

Loading

Laffery Mar 16, 2025 •

edited

Loading

Laffery commented Mar 16, 2025 •

edited

Loading