Refactor getPostLink props #57745
Refactor getPostLink props #57745
Conversation
…editor provider rather than just relying on `template-only` mode
…r usage with `template-only` rendering mode removed
… link not in header as browser back button works
|
Size Change: +165 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
@talldan after using the
This PR suggests a renaming of the setting to So the return value of the the What do you think about this approach. This PR implements this fully across |
| onClick: () => | ||
| changeEntity.goBack(), | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Separating hasHistory from goBack allows us to add these goBack links to nofications prior to moving to the new entity.
There was a problem hiding this comment.
I think I'd need to see an example of it not working to debug it.
My take on it is:
- The function is a closure, so it should be able to access the latest value of
goBackfrom the current scope. - The code in trunk always provides an
onClickhandler, and doesn't make any assumptions about whether the user can go back, so it seems ok not to checkhasHistory.
There was a problem hiding this comment.
Sorry, my explanation was not clear here. The issue is that currently in trunk we only define goBack if there is some history, because we also use it to define that there is history, eg. here in order to work out if the document bar, back links, etc. should appear. So currently when loading /post-template/block-theme.js goBack is undefined because there is no history.
So, in this current setup if we always return goBack as defined without checking history, then in the places where we are using the presence of goBack to indicate history it is always going to be true so we end up with the doc bar and back links in the initial post page, etc.
Combining goBack with hasHistory made sense in the limited use case of the pattern editing, but doesn't extend to the wider use cases, so the two need to be separated. As you note below hasHistory may not be quite the right term, but I think it is ok, and I can't think of a better alternative.
There was a problem hiding this comment.
So, in this current setup if we always return goBack as defined without checking history, then in the places where we are using the presence of goBack to indicate history it is always going to be true so we end up with the doc bar and back links in the initial post page, etc.
The part I don't understand is why goBack always needs to be defined?
There was a problem hiding this comment.
It needs to always be defined, because at this point here when the edit template modal is loaded in the edit post side panel you are still just editing a post, and there is no history, so currently goBack would not be defined so a Go back action can't be added to the success notice.
The only way around this I can see without having goBack always defined would be to trigger the success notice only after you have loaded the template in the editor - which doesn't seem like a good option and not sure how you would actually do that. Or we could just remove the Go back action from these notices as I think this is the only place that it is needed ahead of navigation currently.
|
I'm not sure about the 'entity' naming, as that sounds like it could get confused for a data api. Especially naming like With this API is that it will be important to consider what it will do when the editors are unified, so the naming and functionality should reflect that. I think that's why I'd lean towards more 'router-y' terminology. I'd appreciate some guidance from @youknowriad on this as the person undertaking the majority of the unification work.
This seems a little complicated to me, and I think it might be better just to flatten it into separate settings. Something like: const { getEditPostUrl, navigateToPost, goBack } = getSettings();
const postParams = { postType, postId };
return (
<>
<Button href={ getEditPostUrl( postParams ) } onClick={ () => navigateToPost( postParams ) }>Edit Post</Button>
<Button onClick={ goBack }>Go Back</Button>
</>
);
|
My reason for choosing that was because this is not really about
It didn't seem like it was worth a separate setting for the href, especially since it is only used in one place so far, but I split the As for the Have left it as |
|
Flaky tests detected in 47fb8e6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7496526616
|
glendaviesnz
force-pushed
the
refactor/template-only-mode-in-post-editor
branch
from
8c35c2e
to
78b9d2d
Compare
What?
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast