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.

Sign up for GitHub

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

Jump to bottom

Add test cases for text box underline, strikethrough, and highlight features #311

Merged
merged 3 commits into from
Sep 22, 2024
Merged

Add test cases for text box underline, strikethrough, and highlight features #311

merged 3 commits into from
Sep 22, 2024

Conversation

joemoongit
Copy link
Contributor

Had to make some modifications to lib/recipe/htmlToTextObjects.js and lib/recipe/text.js to make tests pass when

html: true

is set as an option when invoking .text()

julianhille
julianhille reviewed Jun 30, 2023
View reviewed changes
lib/recipe/htmlToTextObjects.js Outdated
@@ -82,19 +82,22 @@ function parseNode(node) {
const parsedData = {
value,
tag: node.tagName,
font: options ? options.font : undefined,
Copy link
Owner

Choose a reason for hiding this comment

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

Are these options in the typescript definitions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These options are not in the typescript definitions. They are similar to the objects that are returned from the function _makeTextObject in lib/receipt/text.js

@julianhille
Copy link
Owner

Thank you very much for the pr

@julianhille
Copy link
Owner

please run prettier on the changed files, prettier is complaining about not compliant files.

see: https://github.com/julianhille/MuhammaraJS/actions/runs/5426565804/jobs/9942641353?pr=311

thanks

@joemoongit
Copy link
Contributor Author

one item to note, when the option

html: true

is set, text wrapped in html tags seem to be created on a new line. i still need to look into this. should i address the fix in this pr? or create another one for it?

@julianhille
Copy link
Owner

i would suggest to start with it and then see. i'd use this as a base branch and then go from there, if its a small change related to this changes add it as commits, if its a complete seperate issue create a PR against this one.

sounds reasonable?

@julianhille julianhille changed the base branch from develop to feature/prepare-v4 July 13, 2023 19:16
@julianhille
Copy link
Owner

I changed base too see if this runs in fixed ci. Would you mind reading on that prepare V4 branch?

@julianhille julianhille deleted the branch julianhille:develop July 13, 2023 22:33
@joemoongit
Copy link
Contributor Author

i don't mind reading from that branch. i see it was deleted though. which branch should i direct this pr to?

@julianhille
Copy link
Owner

Ah. Dang. Sorry. Please reopen against dev.

@julianhille
Copy link
Owner

Test to reopen.

@julianhille julianhille reopened this Jul 14, 2023
@julianhille julianhille changed the base branch from feature/prepare-v4 to develop July 14, 2023 05:08
@joemoongit
Copy link
Contributor Author

I created a commit to address the text wrapped in html tags appearing on a new line. I've also added a test case for bolded text. There are some spacing issues to address now.

after_Add text with underline inside textbox.pdf
before_Add text with underline inside textbox.pdf

julianhille
julianhille reviewed Aug 3, 2023
View reviewed changes
lib/recipe/text.js Outdated
@@ -97,6 +97,7 @@ function _initOptions(self, x = {}, y, options = {}) {
self._firstLineHeight = 0; // indicates not set yet, determined later.
self._textOptions = { textBox: {} };
self._previousTextObjects = [];
self._previousTextObjects2 = [];
Copy link
Owner

Choose a reason for hiding this comment

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

may be there a name which tells a bit more what this variables holds?

@julianhille
Copy link
Owner

sorry this branch has conflicts in the changelog, please rebase / merge dev

@joemoongit
Copy link
Contributor Author

rebased. will work on spacing and may modify the changes a bit more

@joemoongit
Copy link
Contributor Author

joemoongit commented Aug 26, 2024 •
edited
Loading

I will move the changes for the bug fix into its own branch and just keep the tests on this one

@joemoongit joemoongit marked this pull request as ready for review August 28, 2024 16:38
@julianhille
Copy link
Owner

linting has issues

@joemoongit
Copy link
Contributor Author

I ran prettier on this diff, and it says that all files are using the prettier code style. Is there something else I have to do? I am using node v18.17

@julianhille
Copy link
Owner

against what folder? Lint action says: tests/recipe/text.js is needs prettier run

@joemoongit
Copy link
Contributor Author

Ran it on the project's root folder

@julianhille
Copy link
Owner

did you compare versions of prettier?

@joemoongit
Copy link
Contributor Author

ok, that was the issue. it looks like prettier was upgraded to a new major version. one thing though, now that i run prettier, its modifying many more files

@julianhille
Copy link
Owner

I'd suggest to just run it on the one file. Will add a PR fixing all the issues. Otherwise I might have a huge conflict.

@joemoongit
Copy link
Contributor Author

ok, only ran it on the files that were modified on this pr

@julianhille
Copy link
Owner

last thing, i hope, would you mind adding a changelog entry or do you want me to?

  • Added tests for yada yada :P

@joemoongit
Copy link
Contributor Author

i added them under unreleased 😄

@julianhille
Copy link
Owner

i added them under unreleased 😄

im sorry just overlooked it. nevermind. ci run incoming....

@joemoongit
Copy link
Contributor Author

Commit 65159af was run with prettier and node v18.17/npm v9.6.7

Just made a new commit d216ae7 running prettier with node v20.17/npm v10.8.2

@julianhille
Copy link
Owner

🥳 finally a lint run :P

@julianhille
Copy link
Owner

One last thing. Please squash merge fixup some of the last commits and how you'd think it's good readable.

@joemoongit
Copy link
Contributor Author

Squashed

@julianhille julianhille merged commit ee68fb7 into julianhille:develop Sep 22, 2024
273 of 285 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianhille julianhille julianhille approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joemoongit @julianhille