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

newspack-components library #72

Merged
merged 6 commits into from
May 31, 2019
Merged

newspack-components library #72

merged 6 commits into from
May 31, 2019

Conversation

claudiulodro
Copy link
Contributor

@claudiulodro claudiulodro commented May 22, 2019

This PR sets up the ability to publish our components to npm as a stand-alone package. This work is largely based on the two Calypso PRs linked below. Thanks to @ockham for pointing me in the right direction!

To test

  1. In this repo, cd assets/src/components; npm install;.
  2. npm publish --dry-run to see what would be published.
  3. In a separate repo, get the latest version of the npm package (0.1.5+)
  4. Try something like this to verify the components can be used in third-party applications:
import { TextControl, Button, Card } from 'newspack-components';
import { Component, render } from '@wordpress/element';

class PackageTest extends Component {
	render() {
		return (
			<Card>
				<TextControl
					label={'Text Input' }
					value=''
					onChange={ () => {} }
				/>
				<Button isPrimary >
					Button
				</Button>
			</Card>
		);
	}
}

render( <PackageTest />, document.getElementById( 'package-test' ) );

You should see something like this:

Screen Shot 2019-05-24 at 11 21 28 AM

@ockham
Copy link
Contributor

ockham commented May 23, 2019

The issue here lies in the distinction of bundling vs transpilation. Webpack bundles (app) code for use in the browser, i.e. essentially turns import (ES Modules) and require (CommonJS) statements into big concatenated files (bundles), whereas transpilation maps each individual file to a transpiled one -- typically translating ES6+ into ES5. In other words, bundling is the very last step JS code undergoes before being shipped to the browser. (calypso-build's eponymous calypso-build script is essentially a Webpack alias.)

npm packages, by, definition (being Node modules) aren't bundled. They are however expected to be in CommonJS format. This means that import (ES Modules) syntax needs to be transpiled into require. There' a transpile script in calypso-build to do that, which we use in our monorepo for packaging the npms we publish from there (example).

@ockham
Copy link
Contributor

ockham commented May 23, 2019

On a higher level: Great to see an initiative to share components for re-use! As it happens, we've been working on a similar initiative in Calypso (Automattic/wp-calypso#32869, Automattic/wp-calypso#33154).

In order to avoid the proverbial 15 standards situation, I think it'd be great to coordinate. After all, we'd want one canonical source of truth for shared components across Automattic. Maybe we can schedule a Zoom call for stakeholders some time soon?

Looping in @Automattic/team-calypso 🙂

@blowery
Copy link

blowery commented May 24, 2019

@ockham I'd love to be part of that call. /cc @griffbrad

@claudiulodro claudiulodro added [Status] Needs Review The issue or pull request needs to be reviewed and removed Do Not Merge! labels May 24, 2019
@claudiulodro
Copy link
Contributor Author

claudiulodro commented May 24, 2019

I've got things sorted and updated the instructions at the top of the PR. Let's definitely coordinate around components packaging with the Calypso team, as there is definitely some duplicated work we can try and avoid (e.g. we also have a ProgressBar component in this package and a Checklist component, and they look like the Calypso ones).

@claudiulodro claudiulodro changed the title [WIP] newspack-components library newspack-components library May 24, 2019
@justinshreve
Copy link

Awesome work Claudiu! Thank you for working on this. I was able to get this successfully running in our new onboarding work.

Screen Shot 2019-05-27 at 1 08 24 PM

I only had one issue, with the .svg files being loaded directly from CSS files. You may need an appropriate loader to handle this file type.

I had to make sure the following like the following was in our webpack.config.js:

			{
				test: /\.(png|jpe?g|gif|svg|eot|ttf|woff|woff2)$/,
				loader: 'url-loader',
			},

@justinshreve
Copy link

I've noticed a few behaviors with the components that differ from what I see in the Figma Component file. Let me know if you want me to help tackle these.

For inputs, the label should still be displayed when active:

Screen Shot 2019-05-27 at 3 34 32 PM

I also noticed that the active class does not get applied if the input is empty. That means the input looks unfocused even though it is. There should be some kind of active but empty state.

I also noticed that if you tab/keyboard navigate into the inputs, the active class is not appended, even if you begin typing.

@claudiulodro
Copy link
Contributor Author

@justinshreve I've created an issue for what you reported at #75 . If you want to tackle it, that would be super helpful (just assign yourself to the issue), otherwise we'll get it handled soon. Good catch and thanks!

@jeffersonrabb jeffersonrabb self-requested a review May 28, 2019 18:04
Copy link
Contributor

@jeffersonrabb jeffersonrabb left a comment

Choose a reason for hiding this comment

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

The double "src" in the new path structure seems confusing. Could we refactor the components to live in assets/components/src instead of assets/src/components/src? We could also relocate wizards and shared to be siblings of components.

@jeffersonrabb jeffersonrabb added [Status] Needs changes or feedback The issue or pull request needs action from the original creator and removed [Status] Needs Review The issue or pull request needs to be reviewed labels May 28, 2019
@claudiulodro
Copy link
Contributor Author

I've done the requested folder restructuring in 9305614. This should make things cleaner. I'm going to do some research around the best package name for this, then it should be good to merge.

@claudiulodro
Copy link
Contributor Author

Let's keep the current published name (newspack-components). We only have one other software consuming this library, and only temporarily until calypso-ui has form field components. It may even be worth unpublishing this package once that happens, to reduce our maintenance burden. We can discuss that in the future. This should be ready to merge.

@claudiulodro claudiulodro added [Status] Needs Review The issue or pull request needs to be reviewed and removed [Status] Needs changes or feedback The issue or pull request needs action from the original creator labels May 29, 2019
Copy link
Contributor

@jeffersonrabb jeffersonrabb left a comment

Choose a reason for hiding this comment

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

OK, agreed about the package name, and possible plan to unpublish. Perhaps we should add a line to the README to indicate that this is not a stable release and is subject to change at any time, just in case someone we don't know installs and uses the package?

@jeffersonrabb jeffersonrabb added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels May 30, 2019
@claudiulodro
Copy link
Contributor Author

Added disclaimer to readme. 👍

@claudiulodro claudiulodro merged commit 2cb2bc0 into master May 31, 2019
@claudiulodro claudiulodro deleted the add/componentspackage branch May 31, 2019 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants