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

Adds custom spacing to lists #352

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

icopano
Copy link

@icopano icopano commented Nov 12, 2023

Added the possibility to setup a custom spacing to List.
I saw this in a closed issue which was never addressed and thought It would be a nice addition.
This implementation assumes the use of a constant spacing.

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.86%. Comparing base (b7c7e7e) to head (2b49646).
Report is 65 commits behind head on main.

Files with missing lines Patch % Lines
Source/Internal/List/PDFListObject.swift 80.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #352   +/-   ##
=======================================
  Coverage   78.86%   78.86%           
=======================================
  Files         212      212           
  Lines       12607    12617   +10     
=======================================
+ Hits         9942     9950    +8     
- Misses       2665     2667    +2     
Flag Coverage Δ
iOS 79.77% <80.00%> (+<0.01%) ⬆️
macOS 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@philprime
Copy link
Member

Hi @icopano! Sorry for my super late response, busy times and this got lost in my inbox.

Your implementation looks straight away, but I will adapt it slightly, so the flatted() method to find the last element isn't called on every single loop.

@philprime
Copy link
Member

philprime commented Apr 25, 2024

Ok it looks like I can't push to your branch because of branch protections of the main branch (it's often easier to create a new branch after forking).

Instead I pushed my recommended changes in the branch issue-352-adaptions, and I would kindly request you to adapt your PR according to it. I also merged main into it, because I did a lot of changes in the last weeks, and there is a merge conflict.

Commit with my changes: 730bcf3

@philprime
Copy link
Member

Ping @icopano are you still interested in getting this merged?

@icopano
Copy link
Author

icopano commented Nov 13, 2024

Ping @icopano are you still interested in getting this merged?

Hi Philip! Sorry just seen this, I'll have a look at this later this evening, if that's okey with you? Your comment makes sense, thanks for pointing it out!

@philprime
Copy link
Member

No worries, I also forgot about the PR :)

Alright, looking forward to continue working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants