Add: Ability to persist dataviews on the database. #55465
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
oandregal
added
[Type] Feature
oandregal
force-pushed
the
add/saving-to-dataviews
branch
from
e02d1f6
to
e9561e6
Compare
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/experimental/data-views.php ❔ lib/load.php |
|
I've rebased this from Gravacao.do.ecra.2023-10-19.as.10.59.51.movThis is a great start 👏 |
|
I also had some issues with having a view stored and then change the diff --git a/lib/experimental/data-views.php b/lib/experimental/data-views.php
index 8cb6a365d4..b4c82fb860 100644
--- a/lib/experimental/data-views.php
+++ b/lib/experimental/data-views.php
@@ -7,7 +7,7 @@ function _gutenberg_register_data_views_post_type() {
'label' => _x( 'Dataviews', 'post type general name', 'gutenberg' ),
'description' => __( 'Post which stores the different data views configurations', 'gutenberg' ),
'public' => false,
- 'show_ui' => false,
+ 'show_ui' => true,
'show_in_rest' => true,
'rewrite' => false,
'capabilities' => array(And then when to "wp-admin > DataViews" and delete the existing post. |
|
Size Change: +641 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
oandregal
force-pushed
the
add/saving-to-dataviews
branch
from
e9561e6
to
2dd1deb
Compare
|
Rebased with the latest changes and pushed a fix for the Gravacao.do.ecra.2023-10-19.as.14.34.17.mov |
jorgefilipecosta
force-pushed
the
add/saving-to-dataviews
branch
from
8912679
to
cf7c321
Compare
|
Merging this to avoid further conflicts, @oandregal let me know if there is any improvement we can make and I will follow up. |
ntsekouras
reviewed
Oct 20, 2023
| @@ -14,6 +14,9 @@ export default function TextFilter( { filter, view, onChangeView } ) { | |||
| const [ search, setSearch, debouncedSearch ] = useDebouncedInput( | |||
| view.filters[ filter.id ] | |||
| ); | |||
| useEffect( () => { | |||
There was a problem hiding this comment.
I'm not inside how text filters work, maybe @oandregal can clarify why this part is needed?
There was a problem hiding this comment.
This is how to reproduce the bug this effect fixed:
- having a page with title "My page title"
- filter the list of pages by the search component (e.g.: type "title")
- save the view
- reload the page
At this point, we want the search component to show the searched string "title", otherwise the user will see a filtered list but would not know how (or whether) is filtered.
This happens because once Search is mounted, the useDebouncedInput hook is only updated upon changes on local state, ignoring new incoming values for view.search. I've noticed there are two network requests to the pages endpoint:
- the 1st with the default parameters (
view.searchis empty, which is the default state foruseDebouncedInput), - the 2nd with the parameters from the persisted view (
view.searchis "title", the local states needs telling that ).
It seems this code is being refactored. It may be worth taking a look at this after to minimize the network requests, etc.
ntsekouras
reviewed
Oct 20, 2023
| export default function DataviewsProvider( { children } ) { | ||
| const { | ||
| params: { path }, | ||
| } = useLocation(); |
There was a problem hiding this comment.
I don't believe we should couple data views component with the url path. The connection should be made with a param passed to it. How it would work if we have a view with more than one data view components? It's not the current use case, but the component is meant to be reused by consumers to just render data views, so they could have more than one instances in a page and also nothing to do with the url.
There was a problem hiding this comment.
Good point it was addressed at #55695.
mcsf
reviewed
Oct 23, 2023
| } = useLocation(); | ||
| const viewType = PATH_TO_DATAVIEW_TYPE[ path ]; | ||
|
|
||
| if ( window?.__experimentalAdminViews && viewType ) { |
There was a problem hiding this comment.
We need the plugin gate here in addition to the experiment gate, otherwise this can't be tree-shaken.
There was a problem hiding this comment.
Also, we should move the gating out to the module level:
let BlaBla = () => null;
// This condition must stand on its own for correct tree-shaking
if ( process.env.IS_GUTENBERG_PLUGIN ) {
if ( window?.__experimentalAdminViews ) {
BlaBla = DataviewsProvider;
}
}
export default BlaBla;| </DataviewsContext.Provider> | ||
| ); | ||
| } | ||
| export default function DataviewsProvider( { children } ) { |
There was a problem hiding this comment.
Am I correct in assuming that the decision to use a provider is that the context will be managed separately by the main view and the sidebar?
My first impressions: there is a lot of code in this file and I wonder if all the complexity is absolutely necessary.
There was a problem hiding this comment.
Am I correct in assuming that the decision to use a provider is that the context will be managed separately by the main view and the sidebar?
@mcsf Exactly both the sidebar and the main view need to change the active view access the views etc so I needed a provider to share this information/state across different parts of the UI.
there is a lot of code in this file and I wonder if all the complexity is absolutely necessary.
The code contains a lot of complexity, but it is necessary. For example, we need to create a taxonomy that can identify a view for a type such as a page. If the taxonomy does not exist, we need to create it. The logic behind this is that we need to wait for the request to determine if the taxonomy already exists. If it doesn't, we create it, and the same for a post with a default view. Although I considered using actions and resolvers to implement this, it did not seem simple, and I was not sure how to follow that approach without previous history. To make the logic easier to understand, I will add better comments, and I will attempt to reuse more logic between creating a taxonomy and creating a post.
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.
This PR adds the ability to persist the dataviews configuration on the database.
For now in order to make this PR even bigger the UI is not included.
In a follow-up PR, I will include UI to choose between different views, create new views etc...
Testing
Go to the new page list change view settings like number of elements per page sorting etc.
Verify that when you change the view a button "review one change" appears.
Click the button "review one change" Verify you can save the "All" dataview (default view).
Verify that when the view is saved after reload the view has the same settings applied.