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

Added TextStyle Horizontal Alignment #1300

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

visheshdvivedi
Copy link

Added horizontal alignment feature for TextStyle.

Fixes #1282

Checklist:

  • 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

I couldn't find any docstring and a dedicated section as documentation for TextStyle. Please message me if it exists, will update it accordingly

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

@visheshdvivedi visheshdvivedi changed the title Feat textstyle horizontal alignment Added TextStyle Horizontal Alignment Nov 7, 2024
@andersonhc
Copy link
Collaborator

@visheshdvivedi

please remove the "generate=True" from the test below:

Run # Ensuring there is no generate=True left remaining in calls to assert_pdf_equal:
test/outline/test_outline.py: assert_pdf_equal(pdf, HERE / "test_start_section_horizontal_alignment.pdf", tmp_path, generate=True)

@visheshdvivedi
Copy link
Author

Hi @andersonhc,

Added commit to remove generate=True

@andersonhc
Copy link
Collaborator

Hi @andersonhc,

Added commit to remove generate=True

The test on python 3.9 is failing due to use of pipe to union. That feature was introduced in Python 3.10

Can you replace it by Union so we can keep compatibility with older Python versions?

@visheshdvivedi
Copy link
Author

Hi @andersonhc,

Replaced pipe operator with Union as suggested.

Comment on lines +140 to +146
# added support for 'Align' and 'str' type values for l_margin
if isinstance(l_margin, (Align, int)):
self.l_margin = l_margin
elif isinstance(l_margin, str):
self.l_margin = Align.coerce(l_margin)
else:
self.l_margin = 0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# added support for 'Align' and 'str' type values for l_margin
if isinstance(l_margin, (Align, int)):
self.l_margin = l_margin
elif isinstance(l_margin, str):
self.l_margin = Align.coerce(l_margin)
else:
self.l_margin = 0
self.l_margin = Align.coerce(l_margin)

Isn't this one line enough?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @Lucas-C,

I dont think Align.coerce can handle integer values, so I added conditions for the 3 possible types integer, string and Align.

@Lucas-C
Copy link
Member

Lucas-C commented Nov 8, 2024

Good job overall @visheshdvivedi 👍

The GitHub Actions pipeline is currently failing:

Run black --check .
Skipping .ipynb files as Jupyter dependencies are not installed.
You can fix this by running ``pip install "black[jupyter]"``
would reformat /home/runner/work/fpdf2/fpdf2/test/outline/test_outline.py
would reformat /home/runner/work/fpdf2/fpdf2/fpdf/fpdf.py

Oh no! 💥 💔 💥
2 files would be reformatted, 188 files would be left unchanged.

To solve this, you could either install & setup pre-commit hooks: https://py-pdf.github.io/fpdf2/Development.html#pre-commit-hook
Or just run black on the 2 files mentioned in the log above.


Once you'll have rebased your branch on this repo master branch,
you should find this on line 44 in fpdf/html.py:

    "title": TextStyle(  # Only rendered if render_title_tag=True
        b_margin=0.4,
        font_size_pt=30,
        t_margin=6,
        # center=True, - Enable this once #1282 is implemented
    ),

This comes from PR #1298.
Could you replace the commented out center=True line by l_margin=Align.C now that you have implemented this feature, please?

@Lucas-C
Copy link
Member

Lucas-C commented Nov 12, 2024

Hi @visheshdvivedi

Just for your information, several occurences of title_style were renamed into text_style in commit 7b3f1bd last week.

I think this caused minor issues in your PR when you rebased your branch:

fpdf/fpdf.py:5153:31: E0602: Undefined variable 'title_style' (undefined-variable)
fpdf/fpdf.py:5169:11: E0602: Undefined variable 'text_style' (undefined-variable)
fpdf/fpdf.py:5169:26: E0602: Undefined variable 'text_style' (undefined-variable)
fpdf/fpdf.py:5170:20: E0602: Undefined variable 'text_style' (undefined-variable)

@andersonhc andersonhc mentioned this pull request Nov 12, 2024
8 tasks
@Lucas-C
Copy link
Member

Lucas-C commented Nov 20, 2024

Hi @visheshdvivedi 🙂

Just to get a quick update: do you still want to implement this feature?

@visheshdvivedi
Copy link
Author

Hi @Lucas-C ,

Yes, very much.

Aplogies for the delay on my side, have been occupied for sometime in personal tasks. But I will try to put more time on this and complete it.

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.

Feature request: allow horizontal centering in TextStyle
3 participants