Make SaveButton extensible
#56807
Make SaveButton extensible
#56807
Conversation
|
Size Change: +157 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 055dcaf. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7138758765
|
| // For testing | ||
| const { updateSettings } = useDispatch( editSiteStore ); | ||
| useEffect( () => { | ||
| updateSettings( { | ||
| __experimentalSaveButtonAction: () => | ||
| // eslint-disable-next-line no-console | ||
| console.log( 'Customized Action' ), | ||
| __experimentalSaveButtonLabel: 'Customized Action', | ||
| } ); | ||
| }, [ updateSettings ] ); |
There was a problem hiding this comment.
I'll revert this diff after testing. 9929ca0
|
Are you sure this is going to be sufficient? There are more buttons and actions behind this one: 
Will it also be necessary to modify these? Also is there a way for the client to control whether or not the default actions also happen? |
In the context of WordPress.com's needs, the current focus is specifically on customizing the behavior in such a way that the SavePanel (the above UI) does not need to be displayed at all. Therefore, there isn't a requirement to modify other elements like SavePanel in this particular instance.
The current implementation is like a "full customization with full responsibility" approach. This means there's no way to run default actions alongside custom ones. However, in scenarios where such a need arises, clients have the flexibility to invoke Looking ahead, considering potential future complexities with I'm not yet sure if SaveButton needs this flexibility. In my opinion, keeping the current approach and seeing how it adapts to future use cases seems to be okay. What do you think? |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Instead of using settings, would it be better to use applyFilters to allow clients to customize the behavior?
For example,
const label = applyFilters( 'edit-site.SaveButton.label', getLabel(), { isPreviewingTheme, isSaving, ... } );
const onClick = applyFilters(
'edit-site.SaveButton.onClick',
() => setIsSaveViewOpened( true ),
options,
);
Additionally, we can add comments to describe we only want to focus on the SaveButton, and we will provide the customization to the other entry points (e.g.: SavePanel, SaveHub, EntitiesSavedStatesExtensible) in the future if needed.
|
Thank you for your review, @arthur791004!
I did not know that, and it looks much better to me in this case! |
okmttdhr
force-pushed
the
add/save-button-customize
branch
from
7f288c7 to
6c185c6
Compare
okmttdhr
force-pushed
the
add/save-button-customize
branch
from
6c185c6 to
6951167
Compare
okmttdhr
force-pushed
the
add/save-button-customize
branch
from
6951167 to
f01589b
Compare
I changed to use |
| const callback = applyFilters( 'edit-site.SaveButton.onClick', () => | ||
| setIsSaveViewOpened( true ) | ||
| ); | ||
| callback(); |
There was a problem hiding this comment.
I think we can just return the function but it's optional 🙂
| return applyFilters( 'edit-site.SaveButton.label', getLabel(), { | ||
| isSaving, | ||
| disabled, | ||
| isPreviewingTheme, |
There was a problem hiding this comment.
It might be better to add it to the deps as well even if it won't be changed.
But the naming is still confusing as people may not notice it's a function instead of the boolean value. However, it's out of the scope of this PR 🙂
| setIsSaveViewOpened( true ) | ||
| ); | ||
| callback(); | ||
| }, [ setIsSaveViewOpened ] ); |
There was a problem hiding this comment.
Should we also add the dependencies here. Did you get a warning? See #24914
| const callback = applyFilters( 'edit-site.SaveButton.onClick', () => | ||
| setIsSaveViewOpened( true ) | ||
| ); | ||
| callback(); |
There was a problem hiding this comment.
I think we should avoid adding a filter for now. We're still working on unifying the experience between post and site editors and both have different flows, different UIs and different actions. This is going to prevent us from doing this kind of improvement in the future and kind of "freeze" the work.
|
@youknowriad what do you think about the previous approach, using the editor settings? |
|
I don't have enough arguments to block this but it's a bit weird to add these filters, because why not filter every single button to have a different event handler and label. Then maybe filter all the other possible handlers :) All filters once they land are there, as we know, forever. I also don't want to bring the filters are expensive argument 'cause I am not sure this is always true. However, I do wonder if implementers hitting issues like the one described in the PR's description should hook into and filter core, or instead implement a recomposed experience? And if this is impossible the question is why, and where do our components come short in terms of allowing 3rd parties to re-compose different experiences with the same foundation? It feels like the front end is in less need of filters more in need of easy composability ... |
Thanks for your feedback, @youknowriad. I understand the concern about potentially hindering future unification efforts.
Thank you for your insights, @draganescu. 🙂 Could you please elaborate on how you envision "re-compose different experiences"? Currently, without changing the Gutenberg side, the lack of customizability would force implementers to resort to fragile methods like dynamically adding event handlers. I thought that providing a more 'official' to customize components in Gutenberg could improve maintainability and flexibility for developers. |
@okmttdhr I don't really have a vision more like a hope. Briefly put if the experience in core differs from implementation needs, in the browser we should be able to recompose a new preview "screen" based on the same components in core, except the save button. I don't know that this is feasible or possible right away today without automatically creating technical debt in the implementation by diverging from core but it's good I think to aim for this. Ideally, in the client we can repurpose the components that build the client experience. Coming back to this particular problem I wonder if it's not safer to make the whole save button swappable rather than filtering bits of it? This is more inline with the idea of composability. In other words,
... is to me more correct from a birds eye perspective. I am not sure what "overextension for the future use case" means? |
I thought, within Gutenberg, that maintaining a consistent user experience is crucial even if implementers want to change some behavior. The SaveButton, as a primary action within the Site Editor, plays a key role in this experience. So, my idea was that limiting the customizability of key UI would contribute to keeping the "quality" of Gutenberg, rather than allowing them to add any kind of UI. 😃 Also, I can imagine scenarios where the behavior of the SaveButton is changed, such as displaying confetti-like UI or providing guidance after a save action. However, I'm unsure whether there is a use case for completely replacing the UI of the main action button. (In this instance, introducing the Slot/Fill method for adding supplementary UI elements, like those positioned above the SaveButton, might seem appropriate. But using it to completely replace the button might not look ideal to me.) |
There was a problem hiding this comment.
I have been thinking about this and the truth is, I don't have a better alternative to offer, so I'm removing my blocking review. That said, I think there's a couple of things we need to do first:
applyFiltersis not reactive, I think we should build auseFiltershook that works similarly towithFiltersHoC and reacts to filters being removed and added.- I believe to start we should limit these filters to the "Gutenberg plugin" usage only to prevent them to leak to WordPress Core until we decide that this approach is ok to move forward with and support forever.
youknowriad
dismissed
their stale review
No alternative, see previous comment for remaining concerns.
What?
This Pull Request introduces enhancements to the
SaveButtoncomponent, aimed at adding its extensibility within client applications of Gutenberg. Key improvements include the button label, and the action triggered when the button is clicked.Why?
The primary goal of these enhancements is to improve the adaptability of the
SaveButtonwithin client applications, particularly in scenarios where user interaction necessitates a context-specific response.Example
An example of the necessity for these enhancements can be seen on WordPress.com. Say a user is previewing a block theme; if the user does not have the required plan for the previewing theme,
SaveButtonneeds to do more than just save changes. In this case, the button should redirect the user to the plan checkout page (c.f., Automattic/wp-calypso#79223).This example underlines the need for a
SaveButtonthat is not only context-aware but also capable of performing the actions based on the requirements of the client application. By implementing these, we enable a more responsive UX, tailored to the specific conditions of the user's interaction within the client application.How?
The approach taken to enable these enhancements is minimalist, utilizing the existinggetSettings()store.Update: The approach is done by using
applyFiltersin@wordpress/hooks.The Slot/Fill approach was considered but not used. The rationale behind this is that Slot/Fill, while appropriate for replacing entire UI elements, would be an overextension for the future use case. Our aim was to enhance, not replace, the existing
SaveButtonUI.Another considered approach was dynamically adding event listeners to the button (in this case, we don't change anything on the Gutenberg side). However, this was deemed less ideal compared to having a dedicated customization point within the SaveButton itself.
Testing Instructions
Appearance > EditorScreen.Recording.2023-12-08.at.16.53.33.2.mov