-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
DataViews: Add ability to create custom views #55773
Conversation
c88d90d
to
ff85973
Compare
Size Change: +814 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
Flaky tests detected in ff85973. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6722812336
|
When creating a view, I expected the url to change to go to that newly created view. |
I also wonder if we should separate both in the sidebar. (have a title for "custom views" similar to the one we have for "template parts") |
I think we should separate both in the UI and is on of the follow-ups I suggested. I think we can leave for a future PR as this one is already starting to get too big. |
ff85973
to
b678ccf
Compare
Hi @youknowriad, |
b678ccf
to
1e0752c
Compare
@@ -39,19 +44,74 @@ const defaultConfigPerViewType = { | |||
}, | |||
}; | |||
|
|||
export default function PagePages() { | |||
const PATH_TO_TYPE = { |
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.
This const is a bit weird, specially since we're already rendering "page", so we can just use "page" directly.
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.
Done 👍
DEFAULT_VIEWS[ type ].find( ( { slug } ) => slug === activeView )?.view; | ||
const [ view, setView ] = useState( selectedDefaultView ); | ||
|
||
const customViews = useCustomDataViews( type ); |
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.
Not sure I understand why we need this hook, specially since we're calling getEditedEntityRecord
right after.
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.
Oh you're using the slug in the url, why not just the id directly? it would simplify a lot of things IMO
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.
Updated to use the id directly.
); | ||
}, | ||
]; | ||
}, [ editEntityRecord, editedViewRecord?.content, editedViewRecord?.id ] ); |
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 think it's not great that the onChange depend on the value itself. Ideally the customView and setCustomView should be memoized separately.
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.
Updated to memoize separately.
f829850
to
10030d1
Compare
Hi @youknowriad your second round of feedback was applied 👍 |
Squashed commits: [b107c819e2] Feedback application [8ba71aaab4] Post rebase conflicts [13eb412e35] Add: Ability to crate custom DataViews.
10030d1
to
b240331
Compare
As a note old custom views may now crash the application because the view shape was changed. Please delete old views by changing show_ui flags to true in lib/experimental/data-views.php and then using the post manager UI to delete the view posts. Or ignore the old custom views and create new ones. We will need a view versioning system and a way to migrate old view shapes (like we have for blocks and global styles), but right now we are iterating fast, and thinking about view migrations is just something to slow us down. |
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.
This looks good but I think it needs some work design wise on the sidebar:
- custom views need icons
- we need to separate custom and global views.
This PR was missing the type and feature labels for the changelog – added them now. |
|
||
const linkInfo = useLink( { | ||
path, | ||
activeView: isCustom === 'true' ? customViewId : slug, |
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.
It's super confusing to have isCustom
as a string. How did this come to be? Is it because of how params are expected by useLink
? Or is it due to how values are parsed from template records? Either way, shouldn't we be casting to boolean and vice-versa? If not, this prop should probably have a less ambiguous name. /cc @jorgefilipecosta
In this pull request, users can now create their own personalized views, which can be saved and stored in the database. The main focus of this PR is on setting up the necessary mechanisms to add post types and taxonomies, as well as ensuring that the views are correctly referenced and saved to the appropriate entities. However, further UI work will be required in order to distinguish between custom and default views, as well as to enable users to rename or delete their custom views.
Screenshots