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

Refactor/UI list style settings #2028

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

giovanniTramonto
Copy link
Contributor

First commit:
The _updateVisibility function never set any element to display: none. All ui-list (except glyph-search) were using min-height and this.items is defined as array in constructor (should not be undefined). Also I guess, if a list should become hidden, its better practice when the parent decides it (where data is set).

Second commit:
I guess setting min-height for container with custom properties is cleaner.

@justvanrossum
Copy link
Collaborator

The _updateVisibility function never set any element to display: none

I don't think that's true: it is set to none if no minHeight is given, and the items list is empty.

If you don't like the inline style thing that sets min-height, we can still change it to use a variable, but I'd prefer then to have a setter for minHeight that sets this variable, so the other code doesn't need to be changed. The external setting of the CSS variable to me isn't an api improvement.

@giovanniTramonto
Copy link
Contributor Author

Ah, no, you are right, this.items?.length casts to false if 0 : D
But is in fact only used for glyph search. The other implementations always set min-height.
Actually, I don’t care so much for the min-height. It got me because of this display grid style. It looks odd in the stylesheet that grid gets overridden with grid. And I think it’s better to control this in parent where the items are produced.

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.

2 participants