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.
Add Command Palette support #121
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
base: trunk
Are you sure you want to change the base?
Add Command Palette support #121
Changes from all commits
5215215
d6006fe
4ec2af9
18e250f
77bdff1
b8894cf
6fce2a9
f28d69d
6e4431c
65485ac
b06b8a7
f5a6cba
4c6f5da
c592beb
3907d63
60b8184
af46e4a
b2d95ab
101de27
4b6a953
1a62407
47c480c
9bed553
dfa0104
54176c5
a23722c
887f2cf
9944ea2
4def381
1df01fe
ccb0edc
b5f9199
71342ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Personally, I'd argue that we should move away from localized variables like that as much as possible and instead use REST API endpoints. Do SCF has endpoints to retrieve these things, can't we just rely on Core endpoints for post types...?
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 the only endpoints registered are the ones that fetch custom post types and post fields, in every field you can chose to include it or not in the WP default API endpoints for posts fetching, but there are no internal endpoints to retrieve all post types or all post fields created with the plugin.
They are instead localizing the data:
secure-custom-fields/includes/assets.php
Line 603 in 8b71adb
secure-custom-fields/includes/admin/post-types/admin-field-group.php
Line 151 in 8b71adb
Moving everything to a REST API could be done, but this needs to define a new API structure, with its own documentation and decide the approach for the store handling ( using Redux based like wp-data, or move to React Query or something similar )
What's your opinion on that @youknowriad ?
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.
There are two issues why using the Rest API would require some extra work and I didn't consider for this:
acf_get_acf_post_types
), it doesn't have a custom enpoint to do so, just WordPress` built in endpoints.Show in REST API
setting.Therefore, my guess is we will need custom SCF endpoints for the admin that show all, as we will encounter a similar issue when implementing DataViews. What do you think about this approach, Riad?
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.
We're on WordPress, so for me, we should be use the entities and core-data here. no need to duplicate things that exist.
For me this is something that we should try to get rid of. Every CPT should be visible in the endpoints, I think this is a setting of another age.
That said, for now, a custom endpoint/entity could work if we're not ready to make all CPTs created by SCF show_in_rest true. Are there any valid reasons for not doing it?
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.
If by this you mean a custom endpoint, the only reason not to do it right now is the extra complexity it adds to this PR, we should probably tackle that separately.
As per the existing setting, I wouldn't change an existing setting as we don't know all the cases or real users (e.g., having CPTs only for the admin to manage private data), though.
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 is different than
show_in_rest
, this should be mostly handled by thepublic
boolean.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.
Thinking a bit more about this:
types
endpoint.show_in_rest
determines whether a custom post type gets its own endpoint, but also hides the CPT definition from thetypes
endpoint.types
endpoint doesn't return any information to know whether a type is a CPT and/or created by SCF (e.g., nopost-type
field)Therefore, my inclination is to create a new endpoint for custom post types that ignores the
show_in_rest
. This will be needed, too, when migrating the Post Types screen to DataViews.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.
The ideal scenario is to actually use the
types
endpoint and add "metadata" to the post type. That said, I think it's a good interim solution to have a custom endpoint.