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: remove new lines from the end of contextual text blocks #28

Merged
merged 2 commits into from
Jul 20, 2024

Conversation

absorpheus
Copy link
Contributor

@absorpheus absorpheus commented Jul 11, 2024

Summary

Fixes #30

  • Created a utility helper to remove newlines from the end of contextualText text blocks
  • Added tests

This removes newline characters which may exist at the end of contextualText text blocks which are creating unnecessary new lines in Obsidian's editing mode.

This screenshot illustrates the bug in contextualText text blocks using the default template:

contextualText-newline-characters

This is after the fix is applied:

contextualText-after-fix

@absorpheus absorpheus changed the title Bugfix/remove newlines end of contextual text fix: remove newlines end of contextual text Jul 11, 2024
@absorpheus absorpheus changed the title fix: remove newlines end of contextual text fix: remove newlines from the end of contextual text blocks Jul 11, 2024
@absorpheus absorpheus requested a review from bandantonio July 11, 2024 16:42
@bandantonio
Copy link
Owner

Thank you.
I was keeping this issue in mind for a while, and your PR is a good starting point.
In my highlights, I also found the following cases (depends on a book):

  • Trailing spaces - along the way.
  • Newlines and tabs
    • reboot?\n\n
    • project.\n\n\n
    • simple answers.\n\t\t
    • success.\n\t\t\t
    • instead.\n\t\t\t\t\n\t\t\t\t\n\t\t\t\t\t
    • a book\n\t\t\t\t\t\t\n\t\t\t\t\t\n\t\t\t\t\t\n\t\t\t\t\t\t\n\t\t\t\t\t\t\t

So I thought about either making this helper more universal, or merging it (if I/we could find an elegant way) with the existing one that works with indentation: src/methods/aggregateDetails.ts#L51. That's why the issue is still here 😄

@absorpheus absorpheus changed the title fix: remove newlines from the end of contextual text blocks fix: remove new lines from the end of contextual text blocks Jul 11, 2024
@bandantonio bandantonio marked this pull request as draft July 11, 2024 17:49
@bandantonio bandantonio added the bug Something isn't working label Jul 11, 2024
@absorpheus
Copy link
Contributor Author

Thank you for sharing, I wasn't aware that other cases existed! I'm thinking it might be better to keep these helper functions modular so we can easily test them, however it may become cumbersome to call multiple functions nested within each other.

We could have a 'parent' helper function that takes in an array of helper functions and the text as parameters and loops through the helper functions array, invoking the helper function with the outputted result from previous returned text each time.

I'll have a think about it. It would be helpful if we could write some tests so we can better understand the edge cases we are attempting to solve.

@absorpheus
Copy link
Contributor Author

  • Trailing spaces - along the way.

  • Newlines and tabs

    • reboot?\n\n
    • project.\n\n\n
    • simple answers.\n\t\t
    • success.\n\t\t\t
    • instead.\n\t\t\t\t\n\t\t\t\t\n\t\t\t\t\t
    • a book\n\t\t\t\t\t\t\n\t\t\t\t\t\n\t\t\t\t\t\n\t\t\t\t\t\t\n\t\t\t\t\t\t\t

Are these trailing spaces, newlines and tabs appearing only at the end of the highlighted text block (contextualText) or do they appear in other places too? e.g. in between highlighted paragraphs

@bandantonio
Copy link
Owner

Thank you for sharing, I wasn't aware that other cases existed!

No blame at all! I should have created an issue for that.

UPD: Created #30 Let's continue this discussion there and place this PR on hold for some time.

I'm thinking it might be better to keep these helper functions modular so we can easily test them, however it may become cumbersome to call multiple functions nested within each other.

Yes, that's how I thought initially. Modularity is a good approach both from coding and testing perspective, but eventually makes the code base a bit noisy since you end up having several helper functions doing relatively similar actions. Plus, wrapping text fragments into more than two helpers may worsen code readability.

We could have a 'parent' helper function that takes in an array of helper functions and the text as parameters and loops through the helper functions array, invoking the helper function with the outputted result from previous returned text each time.

Yeah, that's one of the possible ways.

I'll have a think about it. It would be helpful if we could write some tests so we can better understand the edge cases we are attempting to solve.

Yes, TDD definitely makes sense.

@bandantonio bandantonio force-pushed the master branch 3 times, most recently from 5f627f2 to e5b071a Compare July 14, 2024 19:59
Copy link
Owner

@bandantonio bandantonio left a comment

Choose a reason for hiding this comment

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

Thank you for the updates.
I left one comment regarding tests, please check it.

Also, please make sure to do rebase of all your commits into one.
Thank you.

test/preserveNewlineIndentation.spec.ts Outdated Show resolved Hide resolved
@bandantonio bandantonio marked this pull request as ready for review July 16, 2024 17:33
src/types.ts Outdated Show resolved Hide resolved
test/mocks/aggregateWrappedTextBlockData.ts Outdated Show resolved Hide resolved
test/mocks/renderedTemplate.ts Outdated Show resolved Hide resolved
absorpheus added a commit that referenced this pull request Jul 17, 2024
* remove unused field from db query response
* make highlight consistent with rest of content
* update test to use the proper highlight template
@absorpheus absorpheus force-pushed the bugfix/remove-newlines-end-of-contextual-text branch from 4c073ec to dcdce55 Compare July 17, 2024 16:11
@absorpheus
Copy link
Contributor Author

absorpheus commented Jul 17, 2024

@bandantonio I've rebased this branch with master and squashed the commits down to one commit.

I force-pushed the branch so the branch's commit history on GitHub is consistent with my local branch's commit history. I think you'll need to fetch this branch again as it's history has been re-written and will be different to your local branch.

Apologies for the inconvenience.

Please let me know if these changes resolve all the suggestions you made.

Thanks

@bandantonio bandantonio force-pushed the bugfix/remove-newlines-end-of-contextual-text branch from 0ed9ccc to 56d3a0c Compare July 20, 2024 23:52
@bandantonio bandantonio merged commit 041d29a into master Jul 20, 2024
6 checks passed
@bandantonio bandantonio deleted the bugfix/remove-newlines-end-of-contextual-text branch July 20, 2024 23:55
@bandantonio bandantonio added this to the ✅ 1.4.0 milestone Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor: make helper functions more universal/modular
2 participants