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

Make TableListLayout respect padding by default #286

Merged
merged 1 commit into from Apr 6, 2021

Conversation

zradke
Copy link
Collaborator

@zradke zradke commented Apr 2, 2021

Previously while you could set padding on the TableListLayout, that
padding would be equally distributed to the left and right rather than
respecting any difference between the two. Even if a custom width was
set on the section or items, if the center alignment was used the
resulting attributes would be globally centered versus centered in the
available width between the padding.

This change resolves the issue on two fronts. First, the
CustomWidth.Alignment.center now center aligns between the provided
padding. Second, because the CustomWidth.default will center align
completely ignoring padding, this change introduces a root CustomWidth
during the layout which all other components merge against to calculate
their origin. This allows the correct center alignment with respect to
padding to cascade down throughout the attributes until a component
overrides the width with something else.

@kyleve
Copy link
Collaborator

kyleve commented Apr 2, 2021

Looking at the updated 200.0 x 700.0.snapshot.png – I think something is a bit wonky. The only spacing provided on that table is the table's entire padding of UIEdgeInsets(top: 10.0, left: 20.0, bottom: 30.0, right: 40.0), but it looks like the rows and section footers are getting inset an extra bit?

Previously while you could set padding on the `TableListLayout`, that
padding would be equally distributed to the left and right rather than
respecting any difference between the two. Even if a custom width was
set on the section or items, if the center alignment was used the
resulting attributes would be globally centered versus centered in the
available width between the padding.

This change resolves the issue on two fronts. First, the
`CustomWidth.Alignment.center` now center aligns between the provided
padding. Second, because the `CustomWidth.default` will center align
completely ignoring padding, this change introduces a root `CustomWidth`
during the layout which all other components merge against to calculate
their origin. This allows the correct center alignment with respect to
padding to cascade down throughout the attributes until a component
overrides the width with something else.
@zradke zradke force-pushed the zradke/fix-padding-in-table-list-layout branch from 63e9ea1 to 59be358 Compare April 2, 2021 19:02
@zradke
Copy link
Collaborator Author

zradke commented Apr 2, 2021

Looking at the updated 200.0 x 700.0.snapshot.png – I think something is a bit wonky. The only spacing provided on that table is the table's entire padding of UIEdgeInsets(top: 10.0, left: 20.0, bottom: 30.0, right: 40.0), but it looks like the rows and section footers are getting inset an extra bit?

Oops, I missed propagating the CustomWidth in a few spots. Should be fixed now.

constraint: layout.width
)
width: layout.width,
alignment: .center
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume / think the actual value passed here doesn't matter, since we're measuring within a container of the same width, right? Eg, no other place we should be pulling this value from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not that I can think of. I didn't see an alignment option in the layout itself. I chose .center because it most closely matched the behavior of CustomWidth.default, with the addition of considering the padding.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@kyleve
Copy link
Collaborator

kyleve commented Apr 2, 2021

Looks great, thank you for fixing! Just one quick last question.

@kyleve
Copy link
Collaborator

kyleve commented Apr 6, 2021

@zradke I added you as a collaborator for this repo, so once you accept you should be able to merge (sorry, didnt realize you weren't one already).

@zradke zradke merged commit 03d2f04 into square:main Apr 6, 2021
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

2 participants