Components: promote ItemGroup
#33701
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciampo
added
[Feature] Component System
|
Size Change: +468 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
…f `ItemGroup` are of the same type (fixes a CSS bug)
ciampo
force-pushed
the
feat/components-promote-item-group
branch
from
88726b5
to
edc4b16
Compare
ciampo
force-pushed
the
feat/components-promote-item-group
branch
from
edc4b16
to
a814b9d
Compare
| <Item>Code is poetry</Item> | ||
| </ItemGroup> | ||
| ); | ||
| expect( wrapper ).toMatchSnapshot(); |
There was a problem hiding this comment.
This should be wrapper.container.firstChild
Suggested change
| expect( wrapper ).toMatchSnapshot(); | |
| expect( wrapper.container.firstChild ).toMatchSnapshot(); |
There was a problem hiding this comment.
Done in 1374e2a (#33701)
Comment on lines
4
to
9
| /** | ||
| * Renders borders around each items. | ||
| * | ||
| * @default false | ||
| */ | ||
| isBordered?: boolean; |
There was a problem hiding this comment.
I think this just renders a border around the entire item group, no? It doesn't affect the items individually I don't think.
There was a problem hiding this comment.
As of now, isBordered renders a border around the entire group and a border in between each item.
This also means that, is isBordered is true, changing the value of isSeparated doesn't
cause any visual changes.
itemgroup-borders-separator.mp4
Should we change the behaviour of isBordered so that it only renders a border around the whole group, and let isSeparated show/hide borders in between each items?
There was a problem hiding this comment.
I actually went ahead in 4c23f28 (#33701) and changed the behaviour of isBordered so that it only affects the border around the item-group (and not the separator in-between items).
Let me know if you find this new behaviour reasonable, or if I should revert back to how it was before
There was a problem hiding this comment.
Should we change the behaviour of
isBorderedso that it only renders a border around the whole group, and letisSeparatedshow/hide borders in between each items?
Yeah! That makes sense!
Comment on lines
17
to
18
| * Renders items individually. Even if `isBordered` is `false`, a border | ||
| * in between each item will be still be displayed. |
There was a problem hiding this comment.
If the above comment is true then this can be shorted to just Renders a separator between each item.
There was a problem hiding this comment.
I like the new proposed description! Assuming the new behaviour for the isBordered prop discussed above, I applied this change in 4c23f28 (#33701)
ciampo
added
[Type] Experimental
|
I also wanted to flag the changes made in |
diegohaz
approved these changes
Jul 28, 2021
| "title": "Item", | ||
| "slug": "item", | ||
| "markdown_source": "../packages/components/src/item-group/item/README.md", | ||
| "parent": "components" |
There was a problem hiding this comment.
I wonder if this should be "parent": "item-group". But I'm not sure if this level of nesting actually works or is desirable in the Handbook. @gziolo Any thoughts?
There was a problem hiding this comment.
The structure in the handbook is rather flat for components. So I would leave it as is.
The other question is whether <Item /> really depends on the <ItemGroup />. In other cases grouping is optional.
There was a problem hiding this comment.
Item is quite dependant on ItemGroup at the moment — for example, it reads the ItemGroup context in order to derive values for the size prop.
On its own, it's not very feature-full, nor opinionated in terms of styling. It really just is a div[role="listitem"] which renders a div or a button depending on the isAction prop.
This was referenced Jul 29, 2021
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
ItemGroupandItemcomponents from thepackages/components/src/uifolder to thepackages/components/srcfolder[first|last]-of-typeselectors where causing an issue when mixing and matching different HTML element types (e.g. when one item hasisAction={true}and the rest of the elements don't). To fix this bug, everyItemis now wrapped in adivcomponent to make sure that all children ofItemGroupare of the same type@wordpress/componentspackageHow has this been tested?
Screenshots
Types of changes
Refactor
Checklist:
*.native.jsfiles for terms that need renaming or removal).