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

GedcomRecord format: Replace HTML in class by view #4660

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jon48
Copy link
Contributor

@jon48 jon48 commented Dec 13, 2022

This is a proposal to replace the HTML hardcoded in the GedcomRecord class for the formatList by a view.
I think this is going in the global direction of managing HTML output by views (and overwriting in subclasses is still possible), and more selfishly, that would allow modules to overwrite the display of this element.

As usual, this is only a proposal, feel free to close it or amend it.

@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Merging #4660 (fef9c8e) into main (a046a25) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##               main    #4660   +/-   ##
=========================================
  Coverage     31.65%   31.65%           
  Complexity    11365    11365           
=========================================
  Files          1163     1163           
  Lines         39520    39516    -4     
=========================================
  Hits          12509    12509           
+ Misses        27011    27007    -4     
Impacted Files Coverage Δ
app/GedcomRecord.php 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jon48 jon48 marked this pull request as ready for review December 13, 2022 20:15
@fisharebest
Copy link
Owner

As far as I can tell, this function is only used by the statistics.

Since it doesn't use any private functions/data of the GedcomRecord object, I don't think it belongs here.

Are you using it in your own code/modules?

@jon48
Copy link
Contributor Author

jon48 commented Dec 23, 2022

I am not calling the method itself in my modules. The only interaction I have with it in in my fork, where I am modifying its body to inject some icons. To improve the compabitility of my modules with the mainstream webtrees, I am looking to move away from those core code modifications, and replace it with the native views overwrite mechanism, and this piece of code is the only one I cannot overwrite (that is why this PR is quite selfish…).

But I agree with you, the method probably does not belong to that class (likely a legacy to old times when a lot of formatting was done in those model classes), and feel free to refactor as you wish, I will adapt to the outcome.

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