Skip to content
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

Support bulk selection by default for every block #406

Open
wants to merge 19 commits into
base: trunk
Choose a base branch
from

Conversation

chriszarate
Copy link
Member

Note

This pull request is large, mostly because the changes it makes affect server-side config schema, query execution, and block bindings as well as client-side rendering, data curation, and data fetching logic. I recommend reviewing by commit.

The initial motivation for this pull request was to revisit a few issues found when reviewing #369:

  • The method used to render multiple results led to non-fatal query execution errors, because the child blocks were not "hydrated" in the expected way:

Screenshot 2025-03-05 at 12 33 50

  • The supports_bulk property could only be successfully applied to ID fields, but there was no enforcement of this.
  • Similarly, the bulk selection implementation within DataViews silently discarded any non-ID query inputs.

These issues were relatively minor, but as @brookewp and I discussed it, we realized that there were some architectural decisions that probably needed revisiting. Doing so would fix the above issues and allow bulk selection for any query type, not just those that opted-in.

They also pave the way for removing "loop" blocks entirely, since both "single" and "loop" blocks are now rendering in the same fundamental way. That work is left for a future PR.

Significant changes in this PR

  • All remote data blocks are now dynamic blocks. Previously, only loop blocks were dynamic—so that they could duplicate their "template" for each result in the remote data set. Now, single blocks also need this capability—since the user can select one or more items.
    • This also allows us to use the render callback function to "hydrate" the remote data in a consistent way, reducing redundant query executions. (Though these were resolved from in-memory cache.)
    • In the future, we might also implement fallback behavior via this render callback function.
  • Allow batch query executions via the QueryRunner and REST API endpoint. This reduces boilerplate in both server-side and client-side code to loop over multiple requests.
    • Update block editor code to support batch queries.
  • Allow multiple / bulk selection for every render query by default. If the query supports an id:list input variable type, it can be resolved by a single query. Otherwise the query is repeated for each selected item.
  • Provide a UUID for each remote data result which can be used as an ID in contexts that require one, like DataViews.
  • Use a consistent response shape for remote data in the block editor. Currently, we simplify and transform the shape of remote data to a flat object in order to make it friendlier for JavaScript code. However, this leads to confusion and loss of type safety. Instead, we can provide utility functions to work with this data.
  • Remain backwards-compatible with blocks that were created prior to this pull request.

That's a lot! The result is more consistency and type safety and better capabilities around batch querying. Here's a short video of multi-select for a block that has done absolutely nothing to opt-in:

Screen.Recording.2025-03-05.at.17.31.30.mov

Known issues

  • Multi-select doesn't currently work with synced patterns and this PR has not fixed that issue. I'll work to fix that separately.

Copy link

github-actions bot commented Mar 5, 2025

Test this PR in WordPress Playground.

@brookewp
Copy link
Contributor

brookewp commented Mar 5, 2025

Thanks for opening this and improving bulk selection! I'm still testing, but I just noticed that it's no longer possible to deselect:

deselect

@chriszarate
Copy link
Member Author

I just noticed that it's no longer possible to deselect

On it! Thanks

@chriszarate
Copy link
Member Author

I just noticed that it's no longer possible to deselect

@brookewp This is fixed in 3dd66d8, but DataViews does not seem to preserve selections across pages even though the underlying state is correct. Is that a known issue?

@brookewp
Copy link
Contributor

brookewp commented Mar 6, 2025

DataViews does not seem to preserve selections across pages even though the underlying state is correct. Is that a known issue?

I have a few related comments about it in #369 after @alecgeatches pointed this out. This is the behaviour in the uncontrolled component, where I don't think the underlying state is correct? It only seems to be possible when using selection and onChangeSelection and controlling the state of the selections ourselves.

But to me, it seems unintentional in the uncontrolled component, and it would be worth checking to see if that's the case or if it's a bug that needs to be reported. So I can follow up with that.

@chriszarate
Copy link
Member Author

It only seems to be possible when using selection and onChangeSelection and controlling the state of the selections ourselves.

But to me, it seems unintentional in the uncontrolled component, and it would be worth checking to see if that's the case or if it's a bug that needs to be reported.

This PR changes the implementation to be a controlled component, see DataViewsModal. The problem is still present, so I think it's an upstream bug.

@brookewp
Copy link
Contributor

brookewp commented Mar 6, 2025

This PR changes the implementation to be a controlled component, see DataViewsModal. The problem is still present, so I think it's an upstream bug.

It is controlled in trunk / from #369, and the selections are preserved there:

Screen.Recording.2025-03-05.at.8.28.33.PM.mov

If this is what you are referring to. I can't replicate this in this branch, so I think some changes removed the behaviour.

@chriszarate
Copy link
Member Author

Silly me, this has nothing to do with DataViews. This is because the UUIDs attached to each result are changing on page navigation because the results are being re-fetched. I will add some memoization to the underlying fetch which will both fix this issue and improve performance. Thanks @brookewp for awesome feedback and detail as always!

@chriszarate
Copy link
Member Author

Now that the remote data results contain type information that was previously stripped out, @brookewp and I agreed that using the first result to introspect the DataViews columns instead of availableBindings made sense.

I've made this change in 14547fd

This effectively means that we are using the output schema of the selection query to display data in DataViews instead of the output schema of the render query.

This also fixes #396 as a side-bonus.

@brookewp
Copy link
Contributor

brookewp commented Mar 7, 2025

It looks like it's not possible to 'Choose' a single item anymore, and results aren't being saved to the front end:

Screen.Recording.2025-03-07.at.12.06.10.PM.mov

I ran into the second issue when implementing bulk selection, but it was just when there were multiple items and not single items, too. I can look back at what happened there and share if it's helpful.

@chriszarate
Copy link
Member Author

It looks like it's not possible to 'Choose' a single item anymore

Thanks! I introduced a regression in one of my fixes. Addressed in 72bd6e4

I also improved the logic so that the actions column is only displayed when relevant: 465a183

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants