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

ListChoice height should be based on content #74

Closed
sjavora opened this issue Mar 23, 2022 · 6 comments · Fixed by #142
Closed

ListChoice height should be based on content #74

sjavora opened this issue Mar 23, 2022 · 6 comments · Fixed by #142
Assignees
Labels
enhancement New feature or request

Comments

@sjavora
Copy link
Member

sjavora commented Mar 23, 2022

From slack:

Height of ListChoice is based on the content. Inner paddings are always the same. Why - to make it easier to develop and understand. We should always stick to the same padding if possible (it wouldn’t make sense to have dynamic padding, it wouldn’t be a systematic approach). You might object - “But different ListChoices can have different sizes then!” - and it’s true but it shouldn’t be a problem as typical list is usually homogenous - all ListChoices are of the same type so they have the same height.

@PavelHolec
Copy link
Collaborator

Isn't this what we already have? We have .small paddings around content, but a strut that makes sure the height is > 44(45)

@sjavora
Copy link
Member Author

sjavora commented Mar 25, 2022

Why is that strut necessary though?

@PavelHolec
Copy link
Collaborator

Without it, the height could be smaller than 44 (it would be 2x12 plus whatever small custom content there is)

@sjavora
Copy link
Member Author

sjavora commented Mar 25, 2022

So as written on slack, that should be 2*12 + 20 (line height) = 44. Are you thinking of some content that's even smaller than that? Do we care about such an eventuality?

@PavelHolec
Copy link
Collaborator

PavelHolec commented Mar 25, 2022

It could happen when using description alone (smaller lineheight), or just any custom content. Even just for the consistency with other tappable components that have minimal width/heigth constraint as a safety measure.

@PavelHolec
Copy link
Collaborator

PavelHolec commented Apr 6, 2022

This task could unify the custom content to use the same layout as in Tile, where caller can still use the heading, while providing the custom content below it.

This might require ListChoice<Content, ValueContent> to support both content below and a value content.

Tile:
image

ListChoice (not possible to combine heading with custom content in typical scenarios):
image

@PavelHolec PavelHolec added the enhancement New feature or request label May 23, 2022
@PavelHolec PavelHolec self-assigned this May 23, 2022
@PavelHolec PavelHolec linked a pull request May 25, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants