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

fix: margin between sibling list items #488

Merged
merged 2 commits into from
Sep 5, 2023
Merged

Conversation

sehyod
Copy link
Collaborator

@sehyod sehyod commented Sep 5, 2023

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
Copy link

codecov bot commented Sep 5, 2023

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
Copy link
Contributor

hammer commented Sep 5, 2023

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
Copy link
Collaborator

@cguedes cguedes left a comment

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
10 checks passed
@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