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

Introduce PluginManager class #19485

Open
wants to merge 81 commits into
base: master
from

Conversation

@ryanwelcher
Copy link
Contributor

ryanwelcher commented Jan 7, 2020

Description

This PR introduces a new PluginManager class that is meant to disconnect the plugin API function calls from direct access to the low-level datatype.

This intermediary makes the API more semantic, allows for future changes/updates more easily and is meant to address @youknowriad 's concerns about the API not being fully semantic.

This PR would be a precursor to changes to the API such as #16384.

As a nice-to-have, I have introduced a new filter to allow the PluginManager class to be filtered. This will allow for ease of testing new API features and underlying data types but is not the main purpose of this PR and is easily removed.

How has this been tested?

This has been tested locally.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
akkspros and others added 14 commits Jan 7, 2020
* added a Text component

* avoid exporting and documenting the mixin

* fix storybook title

* use kebab-case for file naming

* added a _default story with all variants in the one story

* update styles

* update snapshots for react-native

* git and case-sensitive renames are the worst on macos

* git and case-sensitive renames are the worst on macos

* rename tests in kebab-case

* renamed snapshot files

* d'oh forgot to fix the casing ina  file

* update the readme to include when to use the component

* removed eslint disable rule

* update native snapshots

* delete unnecessary tests ready for visual regression testing

* update storyshots

* commit updates to generated file

* renamed Text to __experimentalText

* remove unnecessary files

* Update formatting of typings

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

* fix formatting

* Mark stories as experimental

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

* updated story snapshots

Co-authored-by: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>
…rest of Documentation (#19464)
Removed instances of float:none; in other CSS rules since the component is already set for this.

Remove the float completely.
* WritingFlow: absorb navigation logic from block

* Move selection and navigation logic to WritingFlow

* Use getBlockClientId in useMultiSelection

* Fix block dragging

* Fix focus after escape to nav mode

* Remove IgnoreNestedEvents

* Add getBlockClientId docs

* Restore tabindex comment

* Fix nested block appender click

* Move focus and drag handling

* Create root container for block list

* Polish

* Conditionally bind block event handlers

* Ensure multiselection has 2 items before selecting

* Update packages/block-editor/src/utils/dom.js

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

* Adjust onFocus comment

Co-authored-by: Andrew Duthie <andrew@andrewduthie.com>
* navigation: formatting the allowed styles

* navigation: fix linting errors
@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

ryanwelcher commented Jan 8, 2020

@youknowriad would you be able to review and give feedback here on the approach, please?

WunderBart and others added 7 commits Jan 8, 2020
* Rename LinkControl's "Change" button to "Edit"

* Update LinkControl tests
* useColors: directly pass ref for color detecting

* Remove withFallbackStyles

* Fix useColors parameters

* Use state for detected colors so warning is immediately displayed

* Restore separate targets

* Address feedback
@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Jan 8, 2020

This PR was discussed in Core Editor chat today. See Slack log.

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Jan 8, 2020

Ryan and I chatted about this PR yesterday. My personal observations are:

As a nice-to-have, I have introduced a new filter to allow the PluginManager class to be filtered

  1. I would recommend against adding a new filter as a nice-to-have, and wait until there are compelling use cases.

[emphasis mine] This intermediary makes the API more semantic, allows for future changes/updates more easily

  1. The API itself, as far as users or third-party developers are concerned, remains unchanged: its surface consists of the four top-level functions exposed by @wordpress/plugins. From there I have two comments:
  • If the API remains unchanged, it cannot be said to become more semantic.
  • The choice of data structure or mediator for the data structure doesn’t matter. By “doesn’t matter” I mean that it can be changed at any time with no consequences for the users. However, in this PR we are changing the data structure in order to make future changes hypothetically easier, but we don't yet know what these may be.
  1. On the subject of abstractions:
  • I’m absolutely in favour of adding the right abstractions to add padding for the future. For instance, @wordpress/element was one such abstraction: in the beginning it was a mere membrane for react, but it allowed us to accommodate changes in React, discourage certain patterns or favour others, experiment with Gutenberg-specific idioms, etc.
  • Instead of working on anticipatory changes, I’d focus more on a discussion that can shed light on current limitations of the Plugins API. For instance, as Ryan pointed out, how we can make the API gracefully handle things such as the problems laid out in #16384?

Maybe we can channel the current attention drawn to this PR and to #16384 and attempt to solve a broader problem? It's harder, but it pays off. :)

ediamin and others added 6 commits Jan 14, 2020
* Block: try merging effects

* Merge multi selected effect
…e latest version of npm
…9569)

* [RNMobile] Merge mobile release v1.20.0 back into master (#19562)

* styling fixes after navigation feature merge (#19455)

* Styling fixes to navigation feature

* Add netural styles for toolbar

* Fix condition for not registered component

* Display 'Unsupported' in breadcrumbs for missing components

* Refactor after CR

* Remove leftovers

* [FIX] rich text focus loop (#19240)

* check if onBlur event contains text that is different than value

* add check if there is a text in native event

* Prevent re-selection of RichText when native selection changes as a result of resigning focus

* Fix typo

* Check if isSelected only in onSelectionChangeFromAztec

Co-authored-by: Jorge Bernal <jbernal@gmail.com>

* [RNMobile] Correct displaying photo during upload (#19502)

* [RNMobile] Fix crash once adding Group (#19457)

* Add extra branch for travis to run tests onto

Co-authored-by: Luke Walczak <lukasz.walczak.pwr@gmail.com>
Co-authored-by: Drapich Piotr <drapich.piotr@gmail.com>
Co-authored-by: Jorge Bernal <jbernal@gmail.com>

* Revert travis changes

Co-authored-by: Luke Walczak <lukasz.walczak.pwr@gmail.com>
Co-authored-by: Drapich Piotr <drapich.piotr@gmail.com>
Co-authored-by: Jorge Bernal <jbernal@gmail.com>
@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

ryanwelcher commented Jan 14, 2020

@nerrad thank you for your comments/insight!

I'll remove the filter, it was not required for this PR to function and I agree with the argument of only added it when needed.

To this point:

I think you need to identify what is the broader problem that having a PluginManager class solves? I know "semantics" , "precursor to changes in the API", and "allows for future/updates changes more easily" are some rationale given for the introduction of this class but it still seems a bit vague to me.

This PR is a direct result of the work done for #16384. Specifically this comment. The idea of the API not being semantic enough has been raised a couple of times in Slack in regards to #16384. This PR is an attempt to address that.

As you can imagine, it's starting to feel like we're going in circles a little bit between the two PRs.
While, I'm certainly not looking to rush the correct solution, there just seems to be a lot of push back with very little suggestions as to a course of action that will be acceptable :)

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Jan 18, 2020

I don't know what's happened but my comment has disappeared from this pull it looks like. Also there's still a lot of changes in the changeset for this pull that don't seem related to the pull itself (there's 258 files changed for my view of this pull). What merge strategy is being used? Maybe there's just something awry with github at the time of me posting this.

It's unfortunate I can't see what I originally wrote in here :(

To address your response to the question I had about what problem this pull solves:

This PR is a direct result of the work done for #16384. Specifically this comment. The idea of the API not being semantic enough has been raised a couple of times in Slack in regards to #16384. This PR is an attempt to address that.

I think what I'm trying to point out by my question is that while I understand the inspiration for this pull, I'm not clear on how this specifically addresses what was raised by the comment in #16384. In part, it's likely because it's unclear exactly what @youknowriad is wanting to see. It appears that in this pull, you feel by moving the plain object into a named class PluginManager that this introduces some meaning to the object. I think what @mcsf and I are getting here is we don't see what additional value this brings as there's really not much difference to the api interface.

As you can imagine, it's starting to feel like we're going in circles a little bit between the two PRs.
While, I'm certainly not looking to rush the correct solution, there just seems to be a lot of push back with very little suggestions as to a course of action that will be acceptable :)

To address this pull specifically. I think there's value on pausing the discussion on gets implemented here until:

  • there's a better idea behind what @youknowriad 's had in mind with his comment, since the primary reason for doing the pull was in reaction to that.
  • the conversation happening in #16384 arrives at a agreed upon path forward for the problem it seeks to solve. That in turn might be able to inform whether this pull is needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.