Skip to content
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 FSE opt-in in customizer #307

Merged
merged 27 commits into from
Jun 24, 2022
Merged

Add FSE opt-in in customizer #307

merged 27 commits into from
Jun 24, 2022

Conversation

PatelUtkarsh
Copy link
Collaborator

@PatelUtkarsh PatelUtkarsh commented Feb 14, 2022

Summary

Fixes #255

Todo:

  • Add admin notice to ask users to opt in.
  • Update description.

Demo:

Customizer

fse-opt-in.mp4

Admin notice

notice-updated.mp4

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Contributing Guidelines (updates are often made to the guidelines, check it out periodically).

- Handle customizer preview for opt in or out from fse.
- Load frontend template based on user preference.
- Remove site editor menu by forcing index.html template to disabled.
@PatelUtkarsh PatelUtkarsh marked this pull request as ready for review February 16, 2022 09:18
chunk: 'settings',
entry: [
'./plugin/assets/src/settings/index.js',
'./plugin/assets/css/src/settings.css',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emeaguiar Here theme is referencing to plugin asset - since we are adding setting page in theme as well.

margin-left: auto;
margin-right: auto;
margin-left: auto !important;
margin-right: auto !important;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated: For non FSE it doesn't align center properly when only theme is active.


if ( firstElement ) {
firstElement.setAttribute( 'tabindex', 0 );
if ( drawerElement ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated: Fixes error in some edge case.

return updateFonts();
}

if ( type === UPDATERS.ICONS.type ) {
if ( type === ASSET_UPDATES.ICONS.type ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed reference since now UPDATERS defines object based on global variable and for tests, it fails as it gets empty so using ASSET_UPDATES which UPDATERS uses conditionally.

@PatelUtkarsh
Copy link
Collaborator Author

Hello @emeaguiar, This is ready for another review.

For FSE Opt in now it uses setting page from plugin, If plugin is not active then theme registers setting page and enqueue required assets for settings.

Currently, setting component is shared between plugin and theme both and lives in plugin.

Copy link
Collaborator

@emeaguiar emeaguiar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PatelUtkarsh I just have one suggestion about the settings page. Everything else is great!


const checked = state.updaters.OPT_IN.autoUpdates;

const handleAutoUpdateToggle = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we force a refresh here? I noticed the menus to the site editor either don't appear or remain visible when toggling this, so if I toggle it off and then go to the editor we get an error screen, until we refresh the page

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Updated in 16d33c4

@emeaguiar emeaguiar merged commit a53e5c8 into full-site-editing Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants