-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add the refresh_action #180
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the contribution! I am not convinced yet of the use case, especially since it seems that adding a button action with current url would do the same thing, but I've added some questions in the review.
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 don't think this function is needed, since that identifier is used primarily for the turbo frame. If I call the data_table_name(data_table)
I would expect to retrieve its name (data_table.vars.name
), without any prefix 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.
I understand, but I don't like the idea of having an identification key that is hard-coded in a view and used in several different places.
Do you have any suggestions on how to change this approach? Perhaps a more explicit function name, such as data_table_turbo_identifier
?
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.
Do we need this class (and its interface) at all? I think refreshing the data table should always use the current URL, which (I believe) we can retrieve directly in Twig?
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.
In our project, we sometimes return the same DataTable in multiple views. To avoid instantiating this DataTable in several places (so as to keep the same options), we created an endpoint that returns only the content of this DataTable.
I wanted to replicate this behavior.
public function buildView(ActionView $view, ActionInterface $action, array $options): void | ||
{ | ||
if (ActionContext::Global !== $action->getConfig()->getContext()) { | ||
throw new \LogicException(sprintf('A %s action can only be added as a global action.', $this::class)); |
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.
Good call 👍🏻 It could be used as row or batch action, but that would be unintuitive.
From my point of view, it's not exactly the same. Firstly, it allows the DataTable to be reused in multiple contexts (potentially across several pages) without needing to know the URL to which it should be redirected. It simply redirects to the current URL, directly taking advantage of Turbo to update its content. Secondly, redirecting to the page refreshes the entire page. Imagine a page containing four DataTables; all the data would have to be rebuilt again. Currently, given how Turbo works, that’s also the case (unless I’m mistaken). But we could change this behavior. -> When refreshing a specific DataTable via a query parameter, we could indicate which DT should be refreshed. I might be overthinking it, but I sincerely believe this would be an interesting feature to add to the bundle. What do you think? |
Add a new action, the
RefreshAction
, which, as the name suggests, provides a way to refresh the DataTable's content.This is particularly useful when the data is updated from an external source, and the DataTable serves only as a display.