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

Split handling of HTML attributes & style CSS properties #1211

Merged
merged 3 commits into from
Jun 28, 2024

Conversation

Lucas-C
Copy link
Member

@Lucas-C Lucas-C commented Jun 19, 2024

This PR also:

  • rename the fpdf.TitleStyle class into fonts.TextStyle
  • deprecates tag_indents in favour of tag_styles
  • allows both FontFace and TextStyle instances to be provided as tag_styles
  • remove heading_above & heading_below parameters (not released yet), now handled by .t_margin & .b_margin of HTML tags' TextStyle settings
  • remove list_vertical_margin parameter (not released yet), now handled by .t_margin <ol> & <ul> TextStyle settings
  • removes several hardcoded values in fpdf/html.py

Checklist:

  • The GitHub pipeline is OK (green),
    meaning that both pylint (static code analyzer) and black (code formatter) are happy with the changes of this PR.

  • A unit test is covering the code added / modified by this PR

  • This PR is ready to be merged

  • In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

  • A mention of the change is present in CHANGELOG.md

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

Copy link
Collaborator

@gmischler gmischler left a comment

Choose a reason for hiding this comment

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

Looks reasonable.

Note that svg.py uses parse_style() as well and will need to be adapted.
Also, maybe rename to parse_css_style()?

fpdf/html.py Outdated Show resolved Hide resolved
fpdf/html.py Outdated Show resolved Hide resolved
@Lucas-C Lucas-C force-pushed the html-split-style-and-attrs-handling branch from 9dcea81 to 4a0dab5 Compare June 19, 2024 15:07
@Lucas-C
Copy link
Member Author

Lucas-C commented Jun 19, 2024

Note that svg.py uses parse_style() as well and will need to be adapted. Also, maybe rename to parse_css_style()?

Agreed! Good idea.

CHANGELOG.md Outdated Show resolved Hide resolved
@Lucas-C Lucas-C force-pushed the html-split-style-and-attrs-handling branch 5 times, most recently from 6c8505d to a6e9e9e Compare June 27, 2024 17:26
@Lucas-C Lucas-C force-pushed the html-split-style-and-attrs-handling branch 7 times, most recently from a4cbf6d to cc6df3a Compare June 28, 2024 08:42
@Lucas-C
Copy link
Member Author

Lucas-C commented Jun 28, 2024

Quoting @gmischler in a comment above:

It becomes increasingly clear that all those styling features were slapped together without first trying to come up with a real concept of which role each one is supposed to play. It might be good to take the time now to figure out something a bit more systematic.

There is some truth in this.
What do you mean by "something a bit more systematic"? What interface would you like?

At this point, I think that this PR is "good-enough" to be merged, and we could perform more improvements in later PRs.
Specifically, I'd like to handle most inline HTML tags & most HTML block tags the same way in HTML2FPDF methods.

Still, I'd be happy to merge this, because:

  • it simplifies HTML settings, by deprecating tag_indents in favour of tag_styles, and by getting rid of 3 parameters of write_html() that are now handled by tag_styles
  • it removes several hardcoded values in fpdf/html.py
  • we will never fully support CSS style sheets (with selectors, priority rules, media-queries, etc..), but we still want to provide a way for users to style globally their HTML tags, and IMHO tag_styles seems like a good API that could last

@Lucas-C Lucas-C force-pushed the html-split-style-and-attrs-handling branch from cc6df3a to c7ea250 Compare June 28, 2024 08:56
@Lucas-C
Copy link
Member Author

Lucas-C commented Jun 28, 2024

@lcgeneralprojects : I'd be happy to have your review / feedbacks on this, given that you previously worked on list_vertical_margin in PR #1170

@Lucas-C Lucas-C force-pushed the html-split-style-and-attrs-handling branch from c7ea250 to b18b2fc Compare June 28, 2024 09:00
@Lucas-C Lucas-C added the html label Jun 28, 2024
@Lucas-C Lucas-C force-pushed the html-split-style-and-attrs-handling branch from b18b2fc to 2d09df2 Compare June 28, 2024 09:09
@gmischler
Copy link
Collaborator

What do you mean by "something a bit more systematic"? What interface would you like?

I don't have a very detailled idea of how the optimal API would look like.

There are quite a lof of places that could be equipped with styling (traditional text, HTML tags, table cells/rows/columns, text regions, paragraphs, etc.), all with some common requirements, but also a lot of sometimes subtle differences. My point is that when adding a new option, the way of thinking should always be "could this be used elsewhere? What would need to be different for that?".

It looks like with your changes we're a lot closer to that than before.

we will never fully support CSS style sheets

Never say never... 😉
But yes, for the forseeable future we're very likely going to limit ourselfes to inline CSS. The whole selector business adds a huge amount of complexity, that would probably require full DOM support (both for HTML and SVG).

@Lucas-C Lucas-C merged commit 3b6cd42 into master Jun 28, 2024
13 checks passed
@Lucas-C Lucas-C deleted the html-split-style-and-attrs-handling branch June 28, 2024 21:15
@lcgeneralprojects
Copy link

@lcgeneralprojects : I'd be happy to have your review / feedbacks on this, given that you previously worked on list_vertical_margin in PR #1170

Will try to help, unless it's too late.

@Lucas-C
Copy link
Member Author

Lucas-C commented Jun 28, 2024

Will try to help, unless it's too late.

I merged this PR, but your review is still very welcome!
I you have feedbacks, I'd be happy to include some changes/improvements in my next PR.

@lcgeneralprojects
Copy link

I do like the generalisation and unification of text style management. I also have to say that I am grateful for being mentioned in the changelog.

I won't have time to delve in-depth into the new stuff until at least two more days, but the apparent reduction of 'special cases' seems nice.

@Lucas-C
Copy link
Member Author

Lucas-C commented Jul 1, 2024

Thank you very much for the feedback @lcgeneralprojects! 👍

@Lucas-C
Copy link
Member Author

Lucas-C commented Jul 1, 2024

This PR is a follow-up of the clean-up started there: #1217

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

Successfully merging this pull request may close these issues.

3 participants