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: Remove dops-components as dependency #8208

Merged
merged 70 commits into from
Dec 21, 2017

Conversation

oskosk
Copy link
Contributor

@oskosk oskosk commented Nov 18, 2017

Brings in all the components, mixins, scss, libraries and dependencies that the Admin Page has been using so far from dops-components.

Current status: Everything builds properly. It's a proposal of course.

Changes proposed in this Pull Request:

  • Moves a lot of components from dops-components into _inc/client/components.
  • Moves a lot of css from dops-components into _inc/client/scss.
  • Moves a few mixins from dops-components into _inc/client/mixins.
  • Moves a modules from dops-components into _inc/client/lib.
  • Adds some dependencies to package.json.
  • Removes webpack building instructions for dops-components.
  • Removes dops-components as dependency from package.json and yarn.lock.

Testing instructions:

  • Checkout this branch.
  • Run tests: yarn test-client.
  • Run lint: yarn linst. Expect no errors.
  • Build cleaning all dependencies to be sure: yarn distclean && yarn clean-client && yarn cache clean && yarn build.
  • Build production version cleaning all dependencies to be sure:
     yarn distclean && yarn clean-client && yarn cache clean && yarn
     NODE_ENV=production yarn build-client
    
  • Confirm the Admin Page loads properly.
  • Check that the watch task works: yarn watch and the Admin page loads properly.

Why

  • It's difficult for some folks to just jump in and add the necessary changes to components for the piece of work they're tackling.
  • It's difficult for some folks to have in perspective the other dependants of dops-components in order to be able to contribute to dops-components without breaking the dependants.
  • CSS works and builds well currently but we're accumulating different patterns from different epochs and the first argument ^ helps in those not being up to date.
  • The complexity of having this dependency has been proven for almost two years now.
  • It's hard to make people pay attention to review requests on the dops-components repo.
  • Workflow complexity arising from this dependency does not justify the distributed nature of the components on which Jetpack needs to rely.
  • It's extremely difficult to be able to cope with JS trends and learned best practices if no one feels committed to full-time maintenance of the dependency.
  • Jetpack has some needs for the future that make it quite hard to share the updates needed in dops-components with other dependants that don't have those needs (Compatibility with newer react, with new features of webpack, etc).

@oskosk oskosk requested a review from a team as a code owner November 18, 2017 13:05
@oskosk oskosk added [Status] Proposal [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Nov 18, 2017
@oskosk oskosk force-pushed the remove/dops-components-as-dependency branch from 9f066ab to 90e4eae Compare November 22, 2017 22:47
@dereksmart
Copy link
Member

You hit the nail on the head with the why reasoning behind this. The idea of the separate dependency was well-intentioned, but proved not worth the extra efforts to maintain, especially since it's only us and another product using it.

@dereksmart
Copy link
Member

dereksmart commented Nov 28, 2017

I think we should look at updating the classnames to jetpack- rather than dops-. Probably in a separate PR?

cc @MichaelArestad thoughts?

@MichaelArestad
Copy link
Contributor

I think we should look at updating the classnames to jetpack- rather than dops-. Probably in a separate PR?

@dereksmart Yes. Bingo. That!

Copy link
Member

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

Tests well, I found no regressions. Let's get this in and tidy up the css classNames in a separate PR

@dereksmart
Copy link
Member

Another thought -- We'll want to clean up the imports, switching from external to internal. Can also be handled in another PR

@dereksmart dereksmart merged commit c141f93 into master Dec 21, 2017
@dereksmart dereksmart deleted the remove/dops-components-as-dependency branch December 21, 2017 21:42
@oskosk oskosk added this to the 5.7 milestone Dec 21, 2017
@oskosk oskosk removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Dec 26, 2017
anomiex added a commit that referenced this pull request May 10, 2022
Did a pass over _inc/client/components looking for things that weren't
used anywhere, and found a bunch:

* components/data/query-connect-url - Last use removed in #8014
* components/data/query-connection-status - Last use removed in 62e9ab0
* components/data/query-modules - Last use removed in bfc40ad
* components/data/query-plugin-updates - Last use removed in #17003
* components/data/query-site-products - Last use removed in #21594
* components/form/* - Didn't check for last use, too many bits. But it
  looks like the `formsy-react` package many of these depended on wasn't
  even installed since #8208.
* components/inline-expand - Last use removed in #6550
* components/jetpack-dialogue - Last use removed in #16518
* components/jetpack-logo - Last use removed in #20148
* components/jetpack-termination-dialog - Last use removed in #21048
* components/module-settings/index.jsx - Last use removed in #10644
* components/module-settings/inline-module-toggle.jsx - Last use removed
  in #12118
* components/screen-reader-text - Last use removed in #18843
* components/settings - Last use removed in 26315e1, I think
* components/tags-input - Last use removed in #11772

Then there were a few more that were only used from some of the above:

* components/data/query-connected-plugins
* components/module-settings/form-components.jsx
* components/multiple-choice-question
* components/setting-toggle
anomiex added a commit that referenced this pull request May 10, 2022
Did a pass over _inc/client/components looking for things that weren't
used anywhere, and found a bunch:

* components/data/query-connect-url - Last use removed in #8014
* components/data/query-connection-status - Last use removed in 62e9ab0
* components/data/query-modules - Last use removed in bfc40ad
* components/data/query-plugin-updates - Last use removed in #17003
* components/data/query-site-products - Last use removed in #21594
* components/form/* - Didn't check for last use, too many bits. But it
  looks like the `formsy-react` package many of these depended on wasn't
  even installed since #8208.
* components/inline-expand - Last use removed in #6550
* components/jetpack-dialogue - Last use removed in #16518
* components/jetpack-logo - Last use removed in #20148
* components/jetpack-termination-dialog - Last use removed in #21048
* components/module-settings/index.jsx - Last use removed in #10644
* components/module-settings/inline-module-toggle.jsx - Last use removed
  in #12118
* components/screen-reader-text - Last use removed in #18843
* components/settings - Last use removed in 26315e1, I think
* components/tags-input - Last use removed in #11772

Then there were a few more that were only used from some of the above:

* components/data/query-connected-plugins
* components/module-settings/form-components.jsx
* components/multiple-choice-question
* components/setting-toggle

Co-authored-by: Brandon Kraft <public@brandonkraft.com>
anomiex added a commit that referenced this pull request Jun 6, 2022
These tests were all copied into the repo in #8208 as part of a large
import of code from dops-components. It seems that the testing
infrastructure was never configured to actually run them, and in several
cases necessary supporting files were never copied in.

At this point, 5 years later, it's more likely that someone should
rewrite them from scratch instead of trying to fix these.
anomiex added a commit that referenced this pull request Jun 6, 2022
These tests were all copied into the repo in #8208 as part of a large
import of code from dops-components. It seems that the testing
infrastructure was never configured to actually run them, and in several
cases necessary supporting files were never copied in.

At this point, 5 years later, it's more likely that someone should
rewrite them from scratch instead of trying to fix these.
anomiex added a commit that referenced this pull request Jun 6, 2022
These tests were all copied into the repo in #8208 as part of a large
import of code from dops-components. It seems that the testing
infrastructure was never configured to actually run them, and in several
cases necessary supporting files were never copied in.

At this point, 5 years later, it's more likely that someone should
rewrite them from scratch instead of trying to fix these.
anomiex added a commit that referenced this pull request Jun 7, 2022
These tests were all copied into the repo in #8208 as part of a large
import of code from dops-components. It seems that the testing
infrastructure was never configured to actually run them, and in several
cases necessary supporting files were never copied in.

At this point, 5 years later, it's more likely that someone should
rewrite them from scratch instead of trying to fix these.
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.

None yet

3 participants