Fixes #1757 - Create @clay/label

[bryceosterhaus]

Once 64058ec is merged, I will go through and add additional docs for this component for v2-v3 migration

[coveralls]

Pull Request Test Coverage Report for Build 2187

• 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+1.9%) to 54.212%

---

Totals
Change from base Build 2183:	1.9%
Covered Lines:	130
Relevant Lines:	223

---

💛 - Coveralls

[matuzalemsteles]

I'm finding this component a little tricky and maybe we do not need to call React.createElement directly. We can simplify a little:

Just a suggestion, what do you think? 🙂

const ClayLabelContent = ({
	children,
	closeButtonProps,
}) => (
	<>
		<span className="label-item label-item-expand">{children}</span>
		{closeButtonProps && (
			<span className="label-item label-item-after">
				<button
					{...closeButtonProps}
					className="close"
					type="button"
				>
					{closeButtonProps.children}
				</button>
			</span>
		)}
	</>
);

const ClayLabel = ({
	children,
	className,
	closeButtonProps,
	displayType = 'secondary',
	href,
	large = false,
	...otherProps
}) => {
	const classNames = cN('label', className, {
		'label-dismissible': closeButtonProps,
		'label-lg': large,
		[`label-${displayType}`]: displayType,
	});

	if (href) {
		return (
			<a
				href={href}
                                 className={classNames}
				{...otherProps}
			>
				<ClayLabelContent closeButtonProps={closeButtonProps}>
					{children}
				</ClayLabelContent>
			</a>
		);
	}

	return (
		<span {...otherProps} className={classNames}>
			<ClayLabelContent closeButtonProps={closeButtonProps}>
				{children}
			</ClayLabelContent>
		</span>
	);
}

[bryceosterhaus]

Hey Matu, what exactly do you find confusing? Is it particularly the call to React.createElement? I was trying to avoid multiple returns and also avoiding duplicating the markup for the content.

[matuzalemsteles]

Hey Bryce, yeah I understand what you wanted to do, the big reason is that I do not really like having to call React.createElement directly, I'd rather let the babel presets to react transform to React.createElement, in this case, and I find it a bit strange that we have a condition that removes the href from the above object (it seems a bit hack), I think we have other ways of doing this without having to compromise a little beauty of our code in the development environment.

I think we do not need to focus as much on eliminating as many rows (we can then improve our build steps for distribution) by just getting in the middle so we do not remove some beauty from the code.

Just making sure that we can have other ways of doing this.

[bryceosterhaus]

Good point. I actually was able to simplify it by simply using const TagName = href ? 'a' : 'span'; I think this way makes most sense to me.

[matuzalemsteles]

Oh it looks much better now, good move with TagName.

[matuzalemsteles]

Just started reviewing :)

Sent from GH.

[matuzalemsteles]

hey @bryceosterhaus, since we have a closeButtonProps API I think it makes sense to use ClayIcon directly here to live up to its name and usage.

[bryceosterhaus]

Good call, I didn't think about that. Just updated

[matuzalemsteles]

As we offer the option to pass spritemap to ClayIcon, I think it makes sense to add this API to ClayLabel since we are depending on the Icon, if we go without it, we will force the developer to use only the context.

[bryceosterhaus]

Im a bit worried to go down this path of adding a spritemap prop to every component that we create that consumes icon. One primary reason it seems awkward is because you would naturally expect ClayLabel or anyClay* component to use the icons that come from clay-css. One thought is to automatically include the clay icons within ClayIcon itself, but thats sort of unclear because of the way people host and consume clay-css is different.

Any idea how we could address this? Do you agree its weird to pass spritemap everywhere? My hope was context would fix that problem, but that just means every React app using clay now requires ClayIconContext at the top.

[matuzalemsteles]

One of the problems is that if we do not pass the spritemap to non-React applications (e.g Taglibs), I would not be able to use context and the only way I see it at the moment would be using the spritemap API. Unfortunately I can not think of another way to resolve this outside of that scope.

In taglibs devs do not need to pass the spritemap because we already defined this in the call to the component, but we still need this API, I do not know if we can do something different, in the layers that we will deliver to the Portal we can wrap the components in a type of HOC that uses the context and these layers will have the spritemap API but not ClayLabel for example.

[bryceosterhaus]

yeah thats true about the taglib side. I guess its probably fine to just keep adding spritemap props for now, and then when we get to the other layer to be compatible liferay, we can re-visit it then.

Ill update this PR with spritemap.

[matuzalemsteles]

I had forgotten about this, but wanted us to continue adding the API descriptions here.

[bryceosterhaus]

oh good call, I had forgotten too. I'll send a new PR to update all the existing attributes that were merged and then I will update this PR and any others that I have open.

[matuzalemsteles]

Okay, thanks Bryce! I'll update the clay-link as well.

[bryceosterhaus]

updated

[matuzalemsteles]

hey @bryceosterhaus can you take care of the errors in the CI and add some notes in the migration guide before merge?

[bryceosterhaus]

updated and added docs

[bryceosterhaus]

Any idea how to solve the CI issue where it can't find @clayui/icon?

[wincent]

@bryceosterhaus: it could be that the jest.config.js needs something like this:

  moduleNameMapper: {
    '@clayui/(.+)$': '<rootDir>packages/$1/src',
  },

[matuzalemsteles]

hey @bryceosterhaus and @wincent, I ended up getting these errors too, this is due to the typescript configurations that are trying to find the lib folder of the respective packages, as we do not have this build process in CI will cause these errors, I correct them in PR #1812, I will separate this fix and send it by another PR so that you can give it a rebase over it.

[wincent]

@matuzalemsteles: I find that pretty puzzling. You definitely shouldn't have to do a build in order to run the tests. And the top-level tsconfig doesn't mention lib folders, except to exclude them, right?

[matuzalemsteles]

Yes that is, the issue is that the typescript that is resolving the packages, as we have stated in the typescript configuration to fetch the packages within "@clayui/*": ["./packages/clay-*"] only , it will look for the main field in package.json which is pointing to the lib folder, it is a pity that the typescript settings currently do not allow you to configure it to resolve different main fields, this would be of great help we. I'm crawling this problem at microsoft/TypeScript#21423.

We can go back to the point of pointing directly to the src folder but we will have to add an index.ts file to export the default file.

[wincent]

Thanks for the explanation. That might explain why I didn't run into this difficulty in another monorepo project with TypeScript: there, each package has a "main" of "lib/index.js", and every package has a "src/index.ts", with this tsconfig:

{
  "compilerOptions": {
    "target": "esnext",
    "module": "esnext",
    "moduleResolution": "node",
    "allowJs": true,
    "noEmit": true,
    "strict": true,
    "isolatedModules": true,
    "esModuleInterop": true,
    "baseUrl": "packages",
    "paths": {
      "@my-project/*": ["*/src"]
    }
  },
  "files": ["types/global.d.ts"],
  "include": ["packages/*/src/**/*.ts"]
}

[matuzalemsteles]

Just started reviewing :)

Sent from GH.

[matuzalemsteles]

hey @bryceosterhaus you can take care of the rebase so we can merge.