-
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
Reading Settings: Add 'Your homepage displays' site setting #71550
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~799 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. |
c4a1674
to
13c565b
Compare
1e3e55e
to
355ed45
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.
The PR works as expected and the proposed changes look good to me. 👍🏼👍🏼
I am going to continue reviewing and testing the PR (also on other kinds of sites - not just Simple) once I am back from lunch and wanted to share the initial notes earlier:
- I did not notice any JS, React or TS errors nor warnings.
- The strings for translation look good.
- There's one smaller issue with the usage of
concat()
method (please see the comment in the code below) - current output can be seen in the screenshot below as well - where the em dashes are not separated from the page title. - The page that is selected for the Home page gets disabled properly:
And thank you for ordering multiple elements (typed objects, returned object's properties, etc...) in alphabetical order. 🙂
client/my-sites/site-settings/reading-site-settings/YourHomepageDisplaysSetting.tsx
Outdated
Show resolved
Hide resolved
…pdown options with padding applied;
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.
Tested again after the latest changes. Everything is working as expected. 🙂👍🏼
Page title is now properly separated from dash with a space (for child pages):
Drop-downs display correctly on smaller screens.
Similarly as with other related PR, we were not able to test with Atomic, not Jetpack-connected sites - as /sites/%s/dropdown-pages/
endpoint is not available for them yet.
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7738377 Thank you @mashikag for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Proposed Changes
Display it at the top of the Site settings section
Use the
GET /sites/$site/dropdown-pages/
endpoint to populate the setting's dropdown optionsA page selected in one of the dropdowns should be disabled in another dropdown. E.g.: I have selected About page for Your homepage displays dropdown. Then once I open up the Default posts page dropdown the About page should be disabled.
Display an explanation note as:
For now the "Learn more." link leads to https://wordpress.com/support/templates but there is a new support doc being written up that will replace it when the feature goes out to prod. The following ticket tracks the doc work: https://github.com/Automattic/en.support-docs-content/issues/1805
Testing Instructions
trunk
to your sandboxpublic-api.wordpress.com
http://calypso.localhost:3000/settings/reading/$site
, e.g.:http://calypso.localhost:3000/settings/reading/simple-site.blog
Pre-merge Checklist
Fixes #70409