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

Admin Page: Add module overrides to react UI #8887

Closed
wants to merge 19 commits into from

Conversation

ebinnion
Copy link
Contributor

Fixes #8800

In Progress

@ebinnion ebinnion added Unit Tests Admin Page React-powered dashboard under the Jetpack menu Jetpack Start labels Feb 20, 2018
@ebinnion ebinnion added this to the 5.9 milestone Feb 20, 2018
@ebinnion ebinnion self-assigned this Feb 20, 2018
@ebinnion ebinnion requested a review from a team as a code owner February 20, 2018 17:07
@ebinnion
Copy link
Contributor Author

I just pushed a commit that takes a first pass at adding module override support to the writing tab. It doesn't yet handle infinite scroll or custom content types as those modules have some unique logic. Here are some screenshots for early review:

Lazy Images and Photon forced on:

screen shot 2018-02-20 at 2 08 49 pm

Masterbar, minileven, and post by email forced off:

screen shot 2018-02-20 at 2 08 53 pm

screen shot 2018-02-20 at 2 09 03 pm

Currently, the toggles are disabled, but there is no explanation of why. I'm currently debating adding an explanation at the end of the module description or adding a popover to the disabled toggle. What do you think?

@ebinnion ebinnion added [Status] In Progress [Status] Needs Design Review Design has been added. Needs a review! labels Feb 20, 2018
@oskosk
Copy link
Contributor

oskosk commented Feb 20, 2018

Last time I had to disable a piece of ui for settings I was recommended to hide it instead of disabling (It was the case of the masterbar for AT sites where we force the activation of the module via a filter). #8290 (comment)

@ebinnion
Copy link
Contributor Author

ebinnion commented Feb 20, 2018

I had considered that approach. I went with this approach so far for two reasons:

  1. Devs can completely removed the module using the jetpack_get_available_modules filter.
  2. What happens if a user reads about a setting and expects to see it in their Jetpack admin, but we've completely removed it? I think this would make me feel that Jetpack is broken as opposed to my host configuring the module on/off.

Just removing the modules would make my life a lot easier though ;)

@rickybanister
Copy link

@oskosk I'd asked @ebinnion and crew to provide two different pathways for module activation—one where we fully hide a piece of UI from the interface (so that that code can't run at all OR to ensure a feature is always on behind the scenes without any user interaction) as well as a visible 'always on' or 'always off' mode. This would be desirable in certain situations where a site builder or agency would like to require their clients use or do not use a certain feature.

@ebinnion in this case (the feature being forced into an on or off position) I think the work you did here is perfect. I was going to suggest we add an inline 'info' style notice to each settings card if they are in the 'always on' or 'off' position explaining what's going on, but that may be a bit overbearing. Another solution would be to swap the (i) icon for a warning icon (!) (make the icon red perhaps instead of gray), and use the popover component to display the reason the controls are disabled. Does that make sense?

Something like this:

image

It may be useful to switch the info icon to something like the warning icon to make it more apparent that something has changed.

My copy works well I think (outside of thingy thing), but obviously we'll need to properly explain why the feature is on or off.

@oskosk
Copy link
Contributor

oskosk commented Feb 21, 2018

@rickybanister that's fine. I just wanted to drop a designer opinion about the first time this was done in the admin page.

that was done after having received feedback from a user of an atomic site that just thought the option was there for him to take the masterbar out of his site because we showed him the toggle. Although after seeing the toggle wasn't doing it and was remaining enabled after page refresh, he just thought it was a bug.

@ebinnion ebinnion force-pushed the add/forced-modules-ui-react branch from 2d2ca38 to e3de984 Compare February 23, 2018 18:12
@ebinnion
Copy link
Contributor Author

At this point, toggles and radios should be disabled when overridden for the writing, sharing, and discussion tabs.

@ebinnion ebinnion force-pushed the add/forced-modules-ui-react branch from 421cc13 to 2425d92 Compare February 26, 2018 15:33
@ebinnion
Copy link
Contributor Author

Closing in favor of #8934

@ebinnion ebinnion closed this Mar 27, 2018
@ebinnion ebinnion deleted the add/forced-modules-ui-react branch March 27, 2018 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu Jetpack Start [Status] Needs Design Review Design has been added. Needs a review! Unit Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin Page: Disable/hide setting when module is forced on/off
4 participants