Skip to content

fix: margin between sibling list items#488

Merged
cguedes merged 2 commits into
mainfrom
481-vertical-spacing
Sep 5, 2023
Merged

fix: margin between sibling list items#488
cguedes merged 2 commits into
mainfrom
481-vertical-spacing

Conversation

@sehyod

@sehyod sehyod commented Sep 5, 2023

Copy link
Copy Markdown
Collaborator

There were 2 issues:

  • The spacing between list items was 0 instead of being the same as the spacing between a parent node and its child, resulting in a weird shifting behaviour when indenting/un-indenting an item in a list
  • The spacing between list items was different from the spacing between collapsible nodes.

The first issue was a bug and has been fixed. The second one was more or less intended: the design specifies that there should be 24px between each node:

image

I have changed that to make collapsible nodes considered as list items (which makes sense because that is how we export them in markdown). However, the default spacing remains 24px, which means that turning a regular node into a collapsible node still results in things moving up and down:

Screen.Recording.2023-09-05.at.14.04.12.mov

On notion, the spacing is the same between every kind of node, except headings. So the question is do we want to do the same and remove this 24px spacing we have @hammer?

Fixes #481

@codecov

codecov Bot commented Sep 5, 2023

Copy link
Copy Markdown

Codecov Report

Merging #488 (eb031b4) into main (7792576) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #488      +/-   ##
==========================================
+ Coverage   80.76%   80.78%   +0.01%     
==========================================
  Files         198      198              
  Lines       11856    11864       +8     
  Branches     1141     1141              
==========================================
+ Hits         9576     9584       +8     
  Misses       2266     2266              
  Partials       14       14              
Files Changed Coverage Δ
...components/tipTapNodes/notionBlock/NotionBlock.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hammer

hammer commented Sep 5, 2023

Copy link
Copy Markdown
Contributor

On notion, the spacing is the same between every kind of node, except headings. So the question is do we want to do the same and remove this 24px spacing we have @hammer?

Yes please! The Notion behavior is more natural to me.

@cguedes cguedes self-requested a review September 5, 2023 16:24

@cguedes cguedes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

@cguedes cguedes merged commit f75e256 into main Sep 5, 2023
@cguedes cguedes deleted the 481-vertical-spacing branch September 5, 2023 17:28
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.

Vertical spacing for lists is off

3 participants