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

[MBL-943] Add Additional Header Support to HTML Parser #1844

Merged
merged 2 commits into from Aug 21, 2023

Conversation

msadoon
Copy link
Contributor

@msadoon msadoon commented Aug 17, 2023

πŸ“² What

There's been a change on the web to support a new parser called CKEditor and it supports additional header tags. Previously we supported only h1. Now we support h1, h2, h3, h4, h5, h6.

πŸ€” Why

As stated above, we would eventually need to add new elements/HTML tags supported by more modern project creation tools.

πŸ›  How

Pretty straightforward, taking a look at the Android pull request that does the exact same thing. Used the font sizes for reference so they should be the same size on both platforms.

Might be nice to screenshot test all the supported HTML components one day...thinking when we remove the parser and get good support for story properties on the ProjectFragment we can look into this.

πŸ‘€ See

Before πŸ› After πŸ¦‹
Before πŸ› After πŸ¦‹

βœ… Acceptance criteria

  • Ensure h1 headers are larger size after the change. Before this change they were size 20, now they are 28. Ie. WakeFX Rope-Pal - Automated Waksurf Rope System on staging. (screens above).
  • Ensure new headers are supported via sample staging project "CKE Test project for Mobile". Note: Before adding this support h3 and h4 would not show. HTML snippet from sample project: <h3 id=\"h:Header\" class=\"page-anchor\">Header</h3><h4 id=\"h:Subheader\" class=\"page-anchor\">Subheader</h4>, but now they do. (screens above)

⏰ TODO

  • Update tests.

@msadoon msadoon self-assigned this Aug 17, 2023
@msadoon msadoon added the WIP label Aug 17, 2023
@msadoon msadoon added this to the release-5.10.0 milestone Aug 17, 2023
@msadoon msadoon changed the title [MBL-] Add Additional Header Support to HTML Parser [MBL-943] Add Additional Header Support to HTML Parser Aug 17, 2023
@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #1844 (55d2d28) into main (baffd7c) will increase coverage by 0.01%.
The diff coverage is 94.82%.

@@            Coverage Diff             @@
##             main    #1844      +/-   ##
==========================================
+ Coverage   84.52%   84.53%   +0.01%     
==========================================
  Files        1272     1272              
  Lines      115287   115364      +77     
  Branches    30693    30710      +17     
==========================================
+ Hits        97451    97528      +77     
  Misses      16767    16767              
  Partials     1069     1069              
Files Changed Coverage Ξ”
KsApi/lib/HTML Parser/HTMLParserTests.swift 70.79% <75.00%> (+1.35%) ⬆️
...ib/HTML Parser/Templates/HTMLParserTemplates.swift 98.36% <100.00%> (+0.38%) ⬆️
...ib/HTML Parser/View Elements/ViewElementType.swift 95.65% <100.00%> (+1.20%) ⬆️
...els/HTML Parser/TextViewElementCellViewModel.swift 93.40% <100.00%> (+1.62%) ⬆️
...TML Parser/TextViewElementCellViewModelTests.swift 100.00% <100.00%> (ΓΈ)

πŸ“£ We’re building smart automated test selection to slash your CI/CD build times. Learn more

@msadoon msadoon marked this pull request as ready for review August 17, 2023 04:52
@msadoon msadoon added needs review and removed WIP labels Aug 17, 2023
Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

LGTM other than I see that the headers, World's First Automated Wakesurf Rope System and 100% Automatic to the Surfer and Back to Start are indented unlike the other headers below them.

Could that be an issue with our parser or is that just how the html is initially for this project?

Thanks for linking the android PR. Its good to see that we're maintaining feature parody on with this.

@msadoon
Copy link
Contributor Author

msadoon commented Aug 21, 2023

Not sure what - if any - changes are required here.

@msadoon msadoon merged commit 15d3e2b into main Aug 21, 2023
8 checks passed
@msadoon msadoon deleted the feature/expand-html-parser-header-support branch August 21, 2023 16:33
@scottkicks
Copy link
Contributor

@msadoon meant that to be an approval, not a comment. my bad πŸ˜…

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

Successfully merging this pull request may close these issues.

None yet

2 participants