-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Nav sidebar: Add "view all" and "create new" buttons to sidebar #42605
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~121 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
Caution: This PR affects files in the FSE Plugin on WordPress.com D43887-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2 |
'a8c.WpcomBlockEditorNavSidebar.createPostUrl', | ||
'wpcom-block-editor/getSiteSlug', | ||
( url, postType ) => { | ||
if ( origin && siteSlug && ( postType === 'page' || postType === 'post' ) ) { |
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.
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.
Start up dev environment for calypso, wpcom-block-editor and FSE! 3 seperate apps to make some buttons 🤦
Unfortunately this needs to be tested against calypso.localhost:3000
because this contains changes to the calypsoify iframe. Glad the sidebar is resilient to the new calypso code not being there though! It's falling back to the dotorg site behaviour as expected.
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.
Argh, dang. Thanks for highlighting that. 🤦
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 also wanted to add that the buttons work and look as expected otherwise 👍
83c8a31
to
c532e62
Compare
be9bfde
to
48c0570
Compare
Okay retested using calypso localhost and it works as described. There are no areas in the code that jump out at me, so it would still be good get a onceover from @Automattic/cylon when they have time. |
e118471
to
d578668
Compare
48c0570
to
9609834
Compare
9609834
to
a21d6fa
Compare
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.
so it would still be good get a onceover from cylon
oops
This looks fine from an FSE plugin perspective to me. I didn't test it though :)
Btw, does this handle the existing back button behavior in the top-left of the sidebar thing? i.e. if you enter the site editor from customer home, will it take you back to customer home? (and will the button label be correct?)
...-editing/full-site-editing-plugin/wpcom-block-editor-nav-sidebar/src/components/.eslintrc.js
Outdated
Show resolved
Hide resolved
@noahtallen it goes back to wherever the calypsoify iframe tells us to go back to. So if The label we use for the text link is correct but unfortunately this doesn't fix the tooltip for the (W) button if that's what you meant. I'm going to work on using the official (W) button hook next, which will allow us to fix the tooltip properly. |
Cool. I wasn't even aware that that was broken; I just meant the main label itself. Sounds good! |
IE test failures unrelated: #43298 |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/3851908 Thank you @p-jackson for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Note to anyone reading this while preparing a release of the FSE plugin
This change doesn't require any release notes. We're developing a new feature which is currently hidden behind a flag.
Changes proposed in this Pull Request
a8c.WpcomBlockEditorNavSidebar.allPostsLabel
hook so calypso can override labela8c.WpcomBlockEditorNavSidebar.allPostsUrl
hook so calypso can send user back to calypso rather than wp-admina8c.WpcomBlockEditorNavSidebar.createPostLabel
hook so calypso can override labela8c.WpcomBlockEditorNavSidebar.createPostUrl
hook so calypso will create new post using iframe'd block editora8c.WpcomBlockEditorNavSidebar.linkTarget
hook so calypso can ensure links open in the outer frame ratherI decided to add a single general purpose
getCalypsoUrlInfo
message to<CalypsoifyIframe>
rather than individual messages to get the "view all" and "create new" urls. In the future we'll want the ability to build even more calypso urls, like a block editor url for each post id. Seemed better to just giveiframe-bridge-server.js
the building blocks to do it.I also decided to have messages to fetch the label text from
<CalypsoifyIframe>
rather than have it iniframe-bridge-server.js
. I don't think thewpcom-block-editor
app has support for translations.Testing instructions
WPCOM_BLOCK_EDITOR_SIDEBAR
is defined†wp-env
or something similar to check the FSE plugin works in wp-admin without calypsoify. The button label text might not be perfect; I'm using some pre-existing text that already exists in core. I think it's close enough and we're not expecting this to be used in wp-admin editors initially. It just needs to not break.† If running locally with
wp-env
then defineWPCOM_BLOCK_EDITOR_SIDEBAR
inapps/full-site-editing/.wp-env.override.json
:Or on the sandbox define it in
0-sandbox.php