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

Update block icons to use Gutenberg style instead of Material #724

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

thomasguillot
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

  • Update calypso-build package
  • Remove material-ui packages
  • Add wordpress icons package
  • Use a combination of custom icons and wp icons for the various blocks (inc. toolbar)

How to test the changes in this Pull Request:

  1. Switch to this branch
  2. Test the various Newspack blocks, do you see the new icons

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

* Update calypso-build package
* Remove material-ui packages
* Add wordpress icons package
* Use a combination of custom icons and wp icons for the various blocks (inc. toolbar)
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Maybe this is a node version thing? I'm on v12.21.0. But I'm having major trouble updating my node_modules.

  1. When I try to run npm ci I get the following error:
➜  newspack-blocks git:(update/block-icons) npm ci
npm WARN prepare removing existing node_modules/ before installation
npm ERR! Error while executing:
npm ERR! /usr/local/bin/git ls-remote -h -t ssh://git@github.com/Automattic/wp-prettier.git
npm ERR!
npm ERR! Warning: Permanently added the RSA host key for IP address '140.82.112.3' to the list of known hosts.
npm ERR! git@github.com: Permission denied (publickey).
npm ERR! fatal: Could not read from remote repository.
npm ERR!
npm ERR! Please make sure you have the correct access rights
npm ERR! and the repository exists.
npm ERR!
npm ERR! exited with error code: 128
  1. When manually I delete my .cache folder and node_modules and run npm install, I get this error:
➜  newspack-blocks git:(update/block-icons) ✗ npm i
npm ERR! code ENOLOCAL
npm ERR! Could not install from "node_modules/prettier/parse-srcset@github:ikatyang/parse-srcset#54eb9c1cb21db5c62b4d0e275d7249516df6f0ee" as it does not contain a package.json file.

npm ERR! A complete log of this run can be found in:
npm ERR!     /.npm/_logs/2021-03-25T22_53_25_229Z-debug.log

Have any advice for getting past these errors?

@thomasguillot
Copy link
Contributor Author

Have any advice for getting past these errors?

Hmm I did get a bunch of errors at first too. Ultimately I ended up deleting the entire repo and checking out a fresh master.

Maybe @jeffersonrabb has a better idea?

Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

The errors I ran into were simply because I had an expired GitHub SSH key. Creating a new one fixed the problem. The new NPM packages must have additional dependencies that don't exist in older versions.

A suggestion for potential future refactoring: for the SVGs that don't come from the @wordpress/icons library (the ones with hard-coded SVG code), should we consider moving them to the Newspack Components NPM package so they can be easily imported and reused elsewhere?

@thomasguillot
Copy link
Contributor Author

@dkoo That's a great idea. Maybe we could create a new npm package, e.g. newspack/icons and have a @wordpress/icons dependency so we would only need to install newspack/icons?

@thomasguillot thomasguillot merged commit e8a0eae into master Apr 7, 2021
@thomasguillot thomasguillot deleted the update/block-icons branch April 7, 2021 18:39
@dkoo
Copy link
Contributor

dkoo commented Apr 7, 2021

@thomasguillot I like that idea!

matticbot pushed a commit that referenced this pull request Apr 13, 2021
# [1.24.0-alpha.1](v1.23.0...v1.24.0-alpha.1) (2021-04-13)

### Features

* update block icons to use Gutenberg style instead of Material ([#724](#724)) ([e8a0eae](e8a0eae))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.24.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Apr 13, 2021
# [1.24.0](v1.23.0...v1.24.0) (2021-04-13)

### Features

* update block icons to use Gutenberg style instead of Material ([#724](#724)) ([e8a0eae](e8a0eae))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.24.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

3 participants