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

Desktop, Cli: enex_to_md: support italic in span tags #1966

Merged
merged 5 commits into from
Oct 11, 2019

Conversation

JOJ0
Copy link
Contributor

@JOJ0 JOJ0 commented Oct 8, 2019

Hi, this should theoretically be a no-brainer. It is just an addition to the "span tags format conversion feature" you merged a couple of weeks ago (#1708). I also added a html and md pair test and it passes. Additionally I did a lot of testing with my old Evernote notes.

Anyway, I am experiencing an odd issue: Some tests don't pass and I think I didn't break them:

Error converting file: mathjax_block.html
Error converting file: mathjax_inline.html
Error converting file: table_with_pipe.html

By any chance, are they currently broken or should they work?

Thanks


function isSpanStyleItalic(attributes) {
let style = attributes.style;
if (style.includes('font-style: italic;')) {
Copy link
Owner

Choose a reason for hiding this comment

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

That seems a bit fragile - what if it is set as font-style:italic ;? Maybe spaces should be removed and the string made lowercase before testing it?

Comment on lines 405 to 406
return true;
} else {
return false;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Can be written as return style.includes('...')

@laurent22
Copy link
Owner

Thanks for the PR, overall it looks good, and thanks also for adding the tests.

If some tests are broken they'll need to be fixed, unless they were already broken before your changes. Could you confirm please? Another issue I guess is that the CI is not detecting that any test is failing so that needs to be fixed too, but not something you need to worry about.

@JOJ0
Copy link
Contributor Author

JOJ0 commented Oct 11, 2019

Hi

  • incorporated all your suggestions and amended them to first commit
  • fixed a conflict in CI (because of a now unnecessary debug message)

The thing with tests not running through were my fault: I forgot to rebuild the CliClient and tests! All tests passing now.

@laurent22
Copy link
Owner

Looks good now, thanks for the PR @JOJ0!

@laurent22 laurent22 merged commit f90cc8d into laurent22:master Oct 11, 2019
scoroi pushed a commit to scoroi/joplin that referenced this pull request Nov 10, 2019
* Desktop, Cli: enex_to_md: support italic in span tags

* Desktop, Cli: enex_to_md: readd debug message to resolve CI conflict

* Desktop, Cli: enex_to_md: fix CI errors

add spaces to commented out debug messages

* Desktop, Cli: enex_to_md: remove redundant commented out debug message

* Desktop, Cli: enex_to_md: readd redundant commented out debug message

CI wants it in there - maybe remove in another PR
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.

2 participants