Conversation
|
Size Change: +213 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
noisysocks
reviewed
Jun 6, 2023
Member
There was a problem hiding this comment.
Haven't ran this yet but the code is looking real good. I love the design of Table and a lot of this PR reads like an ad for @wordpress/components – beautiful 🌈.
Not so sure about FilterBar. Seems like a lot of complexity for one filterable attribute. Maybe you know something I don't though e.g that lots more are coming. In my mind though we should just have the pages render a SearchControl.
| ( select ) => | ||
| allTemplateParts?.filter( | ||
| ( template ) => | ||
| ! select( coreStore ).isDeletingEntityRecord( |
Member
There was a problem hiding this comment.
When do we ever want an entity record that is being deleted to be returned by useEntityRecords? I am wondering if we can/should move this logic to within useEntityRecords so as to avoid the awkward useEffect here.
Same comment applies to PageMainTemplates.
Contributor
Author
There was a problem hiding this comment.
This was logic copied over from existing list component. I'm unsure why it was added. @kevin940726 any thoughts on whether this is needed or not?
Member
Contributor
Author
|
@noisysocks thanks so much for the thorough review, I'll look at these tomorrow.
Yeah I probably got a bit carried away here :D I might pull FilterBar out into its own branch for later and use SearchControl for now. |
Contributor
Author
|
@noisysocks should be ready for a re-review. I removed filtering completely to break down the PR a bit. |
|
Flaky tests detected in 4956497. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5212233092
|
kevin940726
reviewed
Jun 8, 2023
Member
kevin940726
left a comment
There was a problem hiding this comment.
Thanks for trying it out! I like the simplicity of the API and the implementation! I left some questions around accessibility and some observations. I'll test it locally next!
| </thead> | ||
| <tbody> | ||
| { data.map( ( row, rowIndex ) => ( | ||
| <tr key={ rowIndex }> |
Member
There was a problem hiding this comment.
Probably not a good idea to use indices as keys?
| export default function Header( { title, subTitle, actions } ) { | ||
| return ( | ||
| <HStack as="header" alignment="left" className="edit-site-page-header"> | ||
| <FlexBlock className="edit-site-page-header__page-title"> |
Member
There was a problem hiding this comment.
Nit: We can use <hgroup> to group the heading with its subheading here. It doesn't seem to have any accessibility impact though so it's just a fun TIL moment and don't need to change 😅 .
| ) | ||
| } | ||
| > | ||
| { templates && <Table data={ templates } columns={ columns } /> } |
Member
There was a problem hiding this comment.
Do we want to render a loading spinner or something like that as a follow-up?
Also for an empty list?
Fixes vertical alignment for custom templates
Contributor
|
Pushed a couple of tiny fixes. Also aligned the contents of the last item in each row |
kevin940726
reviewed
Jun 8, 2023
Member
kevin940726
left a comment
There was a problem hiding this comment.
Tested it locally and LGTM! 👍 💯
Contributor
Author
|
@kevin940726 would you mind taking one final review of this and approving so we can merge :) |
Member
|
Sorry for the late reply! My only concern left is #50766 (comment) which might come back and bite us from the back 😅 . Otherwise this is good stuff! And I'm perfectly okay if we want to handle that issue separately too :). |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
* add page component * hooked up filtering and table * clean up and add new to page component * style updates to templates page * templates page title * template parts using new page component * change page header size * remove tanstack table * remove filter bar for now and simplify code * update package lock * revert package lock * mobile styles for table view * Fix page header z-index * Don't show description wrapper if description is empty Fixes vertical alignment for custom templates * Adjust alignment of last item in each row * fix table padding and scroll --------- Co-authored-by: James Koster <james@jameskoster.co.uk>
16 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
This creates a new Page component along with FilterBar and Table components. I've kept the design mostly the same for this PR so that we can focus a follow up PR on updating design to reflect some of the more recent concepts, including the pattern management grid.
Why?
These components are to be used by pages like the existing templates manage and the upcoming library (replacing template parts). It's the first step towards resolving this issue..
How?
Pagecomponent that accepts title, subTitle, actions and childrenTesting Instructions
Testing Instructions for Keyboard