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

Add Cell Level Controll for Table Borders #1192

Closed
bbrenter opened this issue Jun 4, 2024 · 14 comments · Fixed by #1285
Closed

Add Cell Level Controll for Table Borders #1192

bbrenter opened this issue Jun 4, 2024 · 14 comments · Fixed by #1285

Comments

@bbrenter
Copy link

bbrenter commented Jun 4, 2024

Intent
It would be great to have control over table borders by defining them for each cell.

Describe the solution you'd like
This could for example be implemented by adding the border string slot to the Cell Class and use it inside the get_cell_border function to determine the border for each cell. A possible implementation could look like this...

image

image

image

@andersonhc
Copy link
Collaborator

@bbrenter are you interested in implementing the feature and submitting a PR?

@Kaustbh
Copy link

Kaustbh commented Aug 26, 2024

Hi I'm new to open source, but I'm eager to dive in and help out. I came across this issue and I think I can tackle it. Can I work on it?

@Lucas-C
Copy link
Member

Lucas-C commented Aug 26, 2024

Yes, given the history on this issue, you are very welcome to tackle this @Kaustbh 🙂

As a minor change to the interface suggested by @bbrenter,
I think it would be better if the new border property of Cell was typed with a new subclass of CoerciveEnum, maybe named CellBordersLayout (in a similar fashion as TableBordersLayout). It could have those values:

  • NONE = 0
  • LEFT = 1
  • RIGHT = 2
  • TOP = 4
  • BOTTOM = 8
  • ALL = LEFT | RIGHT | TOP | BOTTOM = 15
  • INHERIT = 16 would be the default value, meaning to inherit from the table layout scheme

A starting point for you @Kaustbh could to be to read our development guidelines.

Then, you could add a new unit test to test/table/test_table.py.

Please ask away if you have any question! 🙂

@Kaustbh
Copy link

Kaustbh commented Aug 29, 2024

Hi @Lucas-C ,

I'm currently working on implementing the CellBordersLayout as suggested. However, I'm having trouble understanding table rendering logic and get_cell_border function, could you please help me?

@Kaustbh
Copy link

Kaustbh commented Sep 9, 2024

I understood the logic of rendering and get_cell_border function, can you please tell me how should I write CellBordersLayout class whether it should be of type CoerciveEnum or CoerciveIntFlag ?

@Kaustbh
Copy link

Kaustbh commented Sep 29, 2024

Please reply on how I should implement CellBordersLayout.

@andersonhc
Copy link
Collaborator

Hi @Kaustbh ,

The CellBordersLayout enum could be a good use case for CoerciveIntFlag if it wasn't for the INHERIT option. INHERIT can't be combined with the other values, so I believe CoerciveEnum is your better option.

@Kaustbh
Copy link

Kaustbh commented Oct 4, 2024

But Enum does not support bitwise operations, which we can do using IntFlag , and please also tell me what should be the input format to the new border parameter in cell() function?

@Kaustbh
Copy link

Kaustbh commented Oct 10, 2024

@Lucas-C can you please tell me the right way to do it?

@Lucas-C
Copy link
Member

Lucas-C commented Oct 11, 2024

Hi @Kaustbh

A quick test revealed that with CoerciveIntFlag as a parent class it works fine:

class CellBordersLayout(CoerciveIntFlag):
    NONE = 0
    LEFT = 1
    RIGHT = 2
    TOP = 4
    BOTTOM = 8
    ALL = 15
    INHERIT = 16

    @classmethod
    def coerce(cls, value):
        if isinstance(value, int) and value > 16:
            raise ValueError("INHERIT cannot be combined with other values")
        return super(cls, cls).coerce(value)

    def __and__(self, value):
        value = super(CellBordersLayout, self).__and__(value)
        if value > 16:
            raise ValueError("INHERIT cannot be combined with other values")
        return value

    def __or__(self, value):
        value = super(CellBordersLayout, self).__or__(value)
        if value > 16:
            raise ValueError("INHERIT cannot be combined with other values")
        return value

print(CellBordersLayout.NONE)
print(CellBordersLayout.BOTTOM)
print(CellBordersLayout.coerce("LEFT"))
print(CellBordersLayout.INHERIT)
print(CellBordersLayout.ALL)
print(CellBordersLayout.LEFT | CellBordersLayout.RIGHT | CellBordersLayout.TOP | CellBordersLayout.BOTTOM)
print(CellBordersLayout.coerce(12))
print(CellBordersLayout.NONE | 1)
print(CellBordersLayout.coerce(17))  # <- this raises an error, as expected
print(CellBordersLayout.INHERIT | 1)  # <- this raises an error, as expected

@Lucas-C
Copy link
Member

Lucas-C commented Oct 14, 2024

Hi @Kaustbh

Did my last answer help you to move forward on this implementation?
Do you have other questions?
Do you plan to continue working on this issue?

@Kaustbh
Copy link

Kaustbh commented Oct 14, 2024

Yes, I am working on this issue, and yes your last answer helped me move forward.

@Lucas-C
Copy link
Member

Lucas-C commented Oct 14, 2024

Yes, I am working on this issue, and yes your last answer helped me move forward.

Great 🙂

I'm assigning this issue to you then!

@Lucas-C
Copy link
Member

Lucas-C commented Dec 16, 2024

This feature has been included in the new release published today:
https://github.com/py-pdf/fpdf2/releases/tag/2.8.2

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

Successfully merging a pull request may close this issue.

4 participants