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

Format notes on display rather than by adding formatting on creation. #4128

Closed
wants to merge 1 commit into from

Conversation

ddrury
Copy link
Contributor

@ddrury ddrury commented Nov 30, 2021

Solves issue #4123 (I think)

To achieve this:

  • Recent changes to the census assistant have been reverted (this obviates the need for retrospective changes to existing notes)
  • Add a new function getHtml() to app\Note.php which returns html formatted according to the tree FORMAT_TEXT preference. Note this is a potentially breaking change as the FORMAT_TEXT preference is strictly applied, currently any note containing markdown table formatting is always interpreted as markdown. However it is simple to revert to the current behaviour (see the comment in app\Note.php line 71)

Because the formatting code is in the Note class, it is necessary to convert in-line notes into temporary dummy note objects so the same code can be used for all notes.

  • Non-markdown formatting just nl2br's the text.
  • For markdown formatting, the new function works by splitting the text into paragraphs (split on "\n" char, tables are treated as a single paragraph) and applying formatting to each paragraph independently and then joining the bits back together. This means that each text paragraph is wrapped in <p></p> and tables are wrapped in <div></div> which allows styling if needed.

Note: some changes have involved classes FunctionsPrint & FunctionsPrintFact that have been slated for removal in 2.1

@codecov
Copy link

codecov bot commented Nov 30, 2021

Codecov Report

Merging #4128 (f6850e6) into main (27a5567) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4128      +/-   ##
============================================
- Coverage     25.87%   25.86%   -0.02%     
- Complexity    11174    11178       +4     
============================================
  Files          1573     1574       +1     
  Lines         48219    48240      +21     
============================================
  Hits          12479    12479              
- Misses        35740    35761      +21     
Impacted Files Coverage Δ
app/Functions/FunctionsPrint.php 0.00% <0.00%> (ø)
app/Functions/FunctionsPrintFacts.php 0.00% <0.00%> (ø)
app/Http/RequestHandlers/ManageMediaData.php 22.78% <0.00%> (ø)
app/Media.php 0.00% <0.00%> (ø)
app/Module/CensusAssistantModule.php 0.00% <0.00%> (ø)
app/Note.php 0.00% <0.00%> (ø)
resources/views/note-page-details.phtml 0.00% <0.00%> (ø)
resources/views/note.phtml 0.00% <0.00%> (ø)
app/SessionDatabaseHandler.php 0.00% <0.00%> (ø)
resources/views/family-page.phtml 0.00% <0.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27a5567...f6850e6. Read the comment docs.

@ddrury ddrury force-pushed the Format_note_on_display branch 3 times, most recently from 5aaa9c7 to 518ffae Compare November 30, 2021 14:26
@ddrury
Copy link
Contributor Author

ddrury commented Nov 30, 2021

BTW, I did think of shifting the view from Functions::printNoteRecord into app/Note::getHtml and by doing so it would make it simple to do away with Functions::printNoteRecord altogether, but it would make app/Note::getHtml quite a lot bigger and I didn't think it was really appropriate for the Note class

Improve regex table detection
Fix change committed in error - Note::getNote()
bug fix - function printMainNotes extracting too much text in some cases
@ddrury ddrury closed this Dec 6, 2021
@ddrury ddrury deleted the Format_note_on_display branch December 7, 2021 16:12
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.

None yet

1 participant