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

Widget Visibility: add controls to widget block editor #20731

Merged
merged 56 commits into from
Sep 23, 2021

Conversation

mreishus
Copy link
Contributor

@mreishus mreishus commented Aug 17, 2021

Fixes #20057

Changes proposed in this Pull Request:

  • Re-implement widget visibility for block based widgets
    • Widget visibility is an existing jetpack feature for old-style widget (non-block based)
    • While creating and editing block based widgets, you may set up visibility rules inside the "Advanced" section of the Gutenberg sidebar
    • While visiting the front-end, widgets will hide and show depending on the rules that have been added
    • Note: While old-style widgets only had visibility rules for each widget, this PR allows visibility rules to be set up for each block within each widget, allowing for complex setups

2021-08-25_14-21
2021-08-23_09-37
2021-08-23_09-37_1

Issues

  • (2) Performance monitoring. mtias asks "Are we monitoring any performance changes this might introduce?"
    • We are not, and I'm currently not sure how to handle this.
    • Is there prior art for doing this?

Issues (Fixed)

  • widget_conditions_data has some nested arrays as options, that render fancy nested selects. Fixed, however it isn't perfect.
    • I was able to make the header options unselectable by disabling them, but gutenberg does not have any components that use <optgroup elements. (which is what the legacy visbility section uses)
  • * Major option "Taxonomy" should be hidden if there's not a non-default taxonomy
  • user and role major options should be hidden on wpcom. Fixed.
  • Does not actually control visibility on the front end. Fixed.
  • Javascript build
    • I'd like to write in JSX, but I haven't found a way. So I'm using element.createElement.
      • Well, I did see instant search, which uses its own preact+webpack+babel setup, but it looked so complicated that I would probably be spending days just on environment setup. babel, package.json and more
    • I am using ES6 features, but I think my code isn't being run by babel, so I might have to convert it to ES5 (this could be tricky with the immutable updates).
    • Implemented Jeremy's build process. Thanks!
  • Technically, legacy widgets are also blocks. This means you can currently specify their visibility on two levels, the legacy level and the new block based one, which would lead to confusing results. Fixed: These are no longer allowed to have controls.
  • Accessibility issues on the delete button
  • On this branch, the /wp-admin/widgets.php page loads with the footer collapsed. There is an uncaught Exception { name: "NS_ERROR_FAILURE", message: "", result: 2147500037, shown in the browser console that's related to this behavior.
    • Fixed here: e00a1f5. It was happening when we were running setAttributes on the core/legacy-widget and core/widget-area blocks, which don't need visibility settings.
  • I cannot add attributes to these types of blocks: 'core/archives', 'core/latest-comments', 'core/latest-posts'. Most blocks let me add a custom attribute, but with these, they start crashing when I add an attribute to them. Why? And are there others that will crash that I haven't found yet?
  • (1) Customizer Bug. F5 -> Visit customizer -> Widgets -> Select a block widget -> Three dots menu -> Show more settings -> Advanced -> Add new rule.
    • Expect to see: A new rule added and you are still looking at the list of rules.
    • Actually see: A new rule added and you are back to looking at the block widget. You must: Three dots menu -> Show more settings -> Advanced -> Add new rule.
    • This bug is non-unique to the code in this branch. It's an existing bug. For example, try: F5 -> Visit customizer -> Widgets -> Select a block widget -> Three dots menu -> Show more settings -> Set the "Font Size" select to Large. Now you're brought back to the block widget in the same manner.
    • This bug is caused by code in core WP.org. Commenting out these 8 lines stops it from happening. They were added in this commit. Comment in trac thread
      • Even though code in WP.org is causing it, it does not happen on a standalone .org installation. This is one of those tricky issues that is happening in an interplay between wp.com and wp.org code.
    • I don't think this bug should be solved in this PR, but elsewhere. Should it be a blocker for this feature?
    • Reported here: Customizer: Block Widgets: Changing settings causes active section to change wp-calypso#55875
    • And on .org trac: https://core.trac.wordpress.org/ticket/54063
    • Created Gutenberg PR which I believe fixes this: Fix Block Settings sidebar unexpectedly collapsing WordPress/gutenberg#34543
    • PR will be released in Gutenberg 11.5, scheduled for release on Sept. 15

Random Musings

  • If you can set visibility rules on any block, it feels like there are so many different places to set them, it could be a potential foot-gun.

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

  • jetpack docker up
  • Make sure WP is installed and you can visit /wp-admin. You may need to use jetpack docker install, and possibly edit one of the env files such that your password isn't easily guessable if you decide to use a tunneling solution in the future.
  • In another terminal, jetpack watch plugins/jetpack
    • You might also need to jetpack build plugins/jetpack first.
  • Make sure this setting is turned on in jetpack settings.
    2021-08-17_17-46
  • Visit http://localhost/wp-admin/widgets.php
  • Add a block based widget here
  • In any block, go to the advanced section of the gutenberg sidebar

2021-08-17_17-39
^ Here it is with no design (although, it might be challenging to fit in sidebar)

Compare to the legacy visibility which we are re-implementing:
2021-08-17_17-40
^ Legacy visibility, not in this PR

Testing on DotCom

  • Sandbox a simple site
  • Apply D65619-code

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello mreishus! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D65619-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@mreishus mreishus self-assigned this Aug 17, 2021
@github-actions github-actions bot added [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Feature] Widget Visibility labels Aug 17, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2021

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ⚠️ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: October 5, 2021.
  • Scheduled code freeze: September 28, 2021.

@github-actions github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Aug 17, 2021
@mreishus mreishus added [Status] In Progress and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Aug 17, 2021
@mreishus
Copy link
Contributor Author

@Automattic/jetpack-design
I'm definitely going to need some help with the design. What I'm doing in this PR is rebuilding the 'widget visibility' settings for block based widgets. This will be placed in the advanced section of the gutenberg sidebar. I believe this means I need to be using gutenberg components, and adhering to their style guidelines as closely as possible.

Here's the current implementation of widget visibility for legacy widgets:
2021-08-17_17-40

It's a rule based setting, where you can add and remove individual rules. Each rule has a major and minor portion. (Example: Major = [page] Minor = [Home page]. Complete rule: "When the current page is the homepage..."). The rules can be linked together with ANDs or ORs. And finally, there's a "Hide" or "Show" action to place.

Here's the result of me haphazardly throwing elements together without a care for the design:
2021-08-17_17-39

Needs a lot of work, and there isn't a lot of space available in the sidebar for complicated controls like these. Any guidance is appreciated, but no rush as I'm still focusing on getting the functionality working.

@mreishus mreishus force-pushed the add/widget-visibility-block-editor branch from 2d74cbd to 55a2ba7 Compare August 18, 2021 19:44
@olaolusoga
Copy link

how does core handle this?

@mreishus
Copy link
Contributor Author

mreishus commented Aug 18, 2021

I added code that recursively filters blocks based on their visibility conditions. For example, you could have a widget that's two columns, then set the right column to only display on certain pages. While this code works fine, it creates blocks that fail to render and crash php sometimes. (Now fixed)

[{"blockName":"core\/columns","attrs":{"conditions":{"action":"show","rules":[],"match_all":"0"}},"innerBlocks":[{"blockName":"core\/column","attrs":{"conditions":{"action":"show","rules":[],"match_all":"0"}},"innerBlocks":[{"blockName":"core\/paragraph","attrs":{"conditions":{"action":"show","rules":[{"major":"page","minor":"front"}],"match_all":"0"}},"innerBlocks":[],"innerHTML":"\n<p>Home Page Only<\/p>\n","innerContent":["\n<p>Home Page Only<\/p>\n"]}],"innerHTML":"\n<div class=\"wp-block-column\"><\/div>\n","innerContent":["\n<div class=\"wp-block-column\">",null,"<\/div>\n"]},{"blockName":"core\/column","attrs":{"conditions":{"action":"show","rules":[],"match_all":"0"}},"innerBlocks":[],"innerHTML":"\n<div class=\"wp-block-column\"><\/div>\n","innerContent":["\n<div class=\"wp-block-column\">",null,"<\/div>\n"]}],"innerHTML":"\n<div class=\"wp-block-columns\">\n\n<\/div>\n","innerContent":["\n<div class=\"wp-block-columns\">",null,"\n\n",null,"<\/div>\n"]}]

^ That's an example $blocks that fails to render. I believe it's because I've wiped out the innerBlocks the column, but the null in the innerContent might mean "Render the next child here", but it no longer exists. So I need a better way to control block visibility rather than just deleting the block.
2021-08-18_18-07

Edit: This is now fixed in 5498fd1

@simison simison requested a review from jeherve August 19, 2021 11:17
@simison
Copy link
Member

simison commented Aug 19, 2021

Some design examples from other plugins at pbAok1-2dI-p2

@mreishus mreishus requested a review from cpapazoglou August 19, 2021 14:15
@cpapazoglou cpapazoglou requested a review from dsas August 19, 2021 14:17
@cpapazoglou
Copy link
Contributor

@dsas could you take a look please cause I will be AFK?

@mreishus mreishus force-pushed the add/widget-visibility-block-editor branch from 0079db6 to 5498fd1 Compare August 19, 2021 15:50
@mreishus
Copy link
Contributor Author

I was able to get the recursive block filtering working on the front-end. Here's a quick demo (turn on sound for commentary):

block-filtering-demo.mp4

@mreishus mreishus requested a review from sixhours August 19, 2021 20:10
@mreishus mreishus force-pushed the add/widget-visibility-block-editor branch from e542aca to 332e837 Compare August 19, 2021 20:13
@mreishus
Copy link
Contributor Author

how does core handle this?

I think core doesn't have a widget visibility feature, and it's always been a jetpack feature. However I'm not certain.

@dsas
Copy link
Contributor

dsas commented Aug 19, 2021

There's an issue when using the customizer to edit the widget on a jetpack site. After setting up a widget with at least one visibility rule and toggling the match all / match one, it goes straight back to the widget list, rather than staying on the edit widget screen. The toggle does take effect though. Here's a short video: https://d.pr/v/DDlJwE

@mreishus
Copy link
Contributor Author

I'm able to replicate this. For me, it's only bringing me back the first time. If I go to advanced and then toggle the match all, it brings me back to the widget list. But if I go back into the same advanced screen, I'm free to toggle it more without being sent back.

https://user-images.githubusercontent.com/937354/130149922-d762b531-f39c-4303-b35d-431a68b08dc3.mp4
^ Toggling "match_all" on jetpack + this branch | First edit sends me back

I've also noticed it happens when I do other settings, like colors:

https://user-images.githubusercontent.com/937354/130149989-ee169b10-62bd-4546-8ec9-f55ce514bd8a.mp4
^ Changing color on jetpack + this branch | First edit sends me back

I also tried it on a production simple site. I found that any edit I made in the advanced section sent me back Additionally, it didn't have the "Only first edit" restriction like I was seeing on jetpack.

https://user-images.githubusercontent.com/937354/130150071-fcd80fe6-f7c5-49d4-b7a7-0a8bd4ad2dde.mp4
^ Changing settings on production simple | All edits send me back

Something it definitely going on. I don't think it's caused by any of this code, but I agree it would make working in the rules area very difficult if it continues to work this way, so perhaps we can identify what it is and how to disable it.

@mreishus
Copy link
Contributor Author

@jeherve Can you help with some build environment questions? In widget-conditions.js, can I use JSX, the spread operator, let/const, and arrow functions? All of these worked for me except JSX, which makes me think that this code isn't being transpiled, and I shouldn't be using these things. I'm guessing I need to remove all ES6 features?

@dsas
Copy link
Contributor

dsas commented Aug 20, 2021

Works identically on a simple site too 👍

I think core doesn't have a widget visibility feature, and it's always been a jetpack feature. However I'm not certain.

This is correct.

@mreishus mreishus force-pushed the add/widget-visibility-block-editor branch from 097a12e to 95ea54e Compare September 23, 2021 14:38
@mreishus
Copy link
Contributor Author

Rebased and reported the above bug in Automattic/wp-calypso#56509 . I think we're ready to proceed here.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good to me, let's merge it! 🚢

@mreishus mreishus merged commit 9d6b2a9 into master Sep 23, 2021
@mreishus mreishus deleted the add/widget-visibility-block-editor branch September 23, 2021 18:57
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D65619-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@mreishus
Copy link
Contributor Author

rWP232223

@mreishus
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Widget Visibility: add controls to widget block editor