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
[lit-virtualizer] Fixes #4505 - Adds documentation for lit-virtualizer/grid #4543
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
📊 Tachometer Benchmark ResultsSummaryA summary of the benchmark results will show here once they finish. ResultsThe full results of your benchmarks will show here once they finish. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JamesIves!!
A couple of small comments from me, but otherwise looks good. @graynorton can you take a quick pass?
packages/labs/virtualizer/README.md
Outdated
|
||
The `gap` property determines spacing between items and can be either a single pixel value such as `8px` (the default), or by applying both a vertical and horizontal pair such as `8px 0`. | ||
|
||
Additionally `auto` can be used to split available space evenly between items, however this may only make sense for inline axis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding "however this may only make sense for inline axis" does auto
even do anything for the block axis? If not, I'd say "however this only works for the inline axis". If it does do something for the block axis it'd be good to say what that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to check on this, but I am running into some issues. I based this particular example on the playground, but it seems like the auto
property may be bugged, or I'm just misunderstanding how it works.
render() {
return html`
<lit-virtualizer
.layout=${grid({
itemSize: {width: '100px', height: '100px'},
gap: '0 auto',
justify: 'space-between'
})}
.items=${this.photos}
.renderItem=${photos => html`<img src=${photo.url}>`}
></lit-virtualizer>
`;
}
The following produces this error: Error: grid layout: column-gap cannot be set to 'auto' when direction is set to vertical
, but from what I can tell direction
doesn't actually work for a grid layout, as changing it at all causes the virtualizer to disappear.
Perhaps I should omit documenting the auto property for now? Regardless I've added some additional context surrounding the justify
requirement which was missing before.
packages/labs/virtualizer/README.md
Outdated
|
||
#### padding | ||
|
||
The `padding` property determines the spaces around the edges and can either be a single value pair such as `8px 16px`, or four values to specify top, right, bottom, and left independently, for example `8px 12px 16px 24px`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"around the edges" around the edges of items? If so I'd say that, and if possible clarify the difference between padding and gap. Maybe "around each item" would be enough to imply the difference and most people should be familiar with the concepts from CSS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that change 👍🏻
Bumpity bump @graynorton |
Adds documentation for
lit-virtualizer
grid
layout.Documentation was produced using examples from the Playground / code. Please let me know if you feel I need to go into more detail in any specific area.
Closes #4505