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

feat: Add box-sizing:border-box automatically to all styles created by createStyles #2721

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions modules/codemod/lib/v11/spec/replaceStylesIconProp.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('replaceStylesIconProp', () => {
<AccentIcon styles={{padding: '1rem'}} />
</>
`;
expectTransform(input, expected); //?
expectTransform(input, expected);
});

it('should rename styles to cs for Svg, SystemIcon, AccentIcon exported from the icon package', () => {
Expand Down Expand Up @@ -75,7 +75,7 @@ describe('replaceStylesIconProp', () => {
<AccentIcon cs={{padding: '1rem'}} />
</>
`;
expectTransform(input, expected); //?
expectTransform(input, expected);
});

it('should handle value as variable', () => {
Expand Down
49 changes: 24 additions & 25 deletions modules/docs/lib/InformationHighlight/index.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from 'react';
import {createContainer} from '@workday/canvas-kit-react/common';
import {createModifiers, createStyles, cssVar} from '@workday/canvas-kit-styling';
import {createStencil, cssVar} from '@workday/canvas-kit-styling';
import {base, system} from '@workday/canvas-tokens-web';

import {Base, BaseProps} from './Base';
Expand All @@ -16,29 +16,28 @@ interface InformationHighlightProps extends BaseProps {
variant?: Variant;
}

const containerStyles = createStyles({
backgroundColor: base.soap100,
display: 'grid',
gridTemplateColumns: 'min-content',
columnGap: system.space.x4,
rowGap: system.space.x2,
padding: system.space.x4,
borderRadius: system.shape.x1,
});

const containerModifiers = createModifiers({
variant: {
emphasis: createStyles({
borderInlineStart: `solid ${cssVar(system.space.x1)} ${cssVar(base.blueberry400)}`,
}),
caution: createStyles({
borderInlineStart: `solid ${cssVar(system.space.x1)} ${cssVar(
base.cantaloupe400
)}`,
}),
attention: createStyles({
borderInlineStart: `solid ${cssVar(system.space.x1)} ${cssVar(base.cinnamon500)}`,
}),
const informationHighlightStencil = createStencil({
base: {
backgroundColor: base.soap100,
display: 'grid',
gridTemplateColumns: 'min-content',
columnGap: system.space.x4,
rowGap: system.space.x2,
padding: system.space.x4,
borderRadius: system.shape.x1,
},
modifiers: {
variant: {
emphasis: {
borderInlineStart: `solid ${cssVar(system.space.x1)} ${cssVar(base.blueberry400)}`,
},
caution: {
borderInlineStart: `solid ${cssVar(system.space.x1)} ${cssVar(base.cantaloupe400)}`,
},
attention: {
borderInlineStart: `solid ${cssVar(system.space.x1)} ${cssVar(base.cinnamon500)}`,
},
},
},
});

Expand All @@ -55,7 +54,7 @@ export const InformationHighlight = createContainer('section')({
return (
<Base
as={Element}
cs={[containerStyles, containerModifiers({variant: model.state.variant})]}
{...informationHighlightStencil({variant: model.state.variant})}
Copy link
Member

Choose a reason for hiding this comment

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

While a stencil does return a spreadable object, this pattern doesn't handle merging. There's a very simple way to break InformationHighlight:

<InformationHighlight className="whoops" />

The className prop returned by the stencil will be overwritten by the incoming prop. At this point, Base isn't doing anything useful for us.

This is also why I'm not a fan of us using a Base component because even though we can use the cs prop, our component cannot handle the cs prop because it will get overridden instead of merged. I think the safest way these components work is by using handleCsProp and dropping Base:

return (
  <Element {...handleCsProp(props, informationHighlightStencil({variant: model.state.variant}))} />

{...props}
/>
);
Expand Down
37 changes: 36 additions & 1 deletion modules/docs/mdx/11.0-UPGRADE-GUIDE.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ any questions.
- [Text](#text)
- [Style Utility Updates](#style-utility-updates)
- [createStencil](#createstencil)
- [createStyles](#createstyles)
- [Glossary](#glossary)
- [Main](#main)
- [Preview](#preview)
Expand Down Expand Up @@ -231,7 +232,9 @@ instead.

**PR:** [#2472](https://github.com/Workday/canvas-kit/pull/2700)

As part of the refactor, we've removed the following exports that were primarily used to style the component:
As part of the refactor, we've removed the following exports that were primarily used to style the
component:

- `useStatusIndicatorModel`
- `useStatusIndicator`
- `statusIndicatorColors`
Expand Down Expand Up @@ -767,6 +770,7 @@ our
The component now supports the `cs` prop, but otherwise the API has not been changed.

> #### Visual Breaking Change
>
> Opacity applied to the whole container has been removed for transparent variant and replace by
> translucent color. By this change opacity has been changed from `0.85` to `0.84`, so there is
> possibility of the small visual change.
Expand Down Expand Up @@ -811,13 +815,44 @@ they extend the `Text` component. These changes do not affect the components API

## Style Utility Updates

### `createModifiers`

`createModifiers` has been updated to accept styles as a style object. Previosly it accepted only a
string value generated by `createStyles`.

```ts
// before
const myModifiers = createModifiers({
size: {
small: createStyles({fontSize: 12}),
medium: createStyles({fontSize: 14}),
},
});

// now
const myModifiers = createModifiers({
size: {
small: {fontSize: 12},
medium: {fontSize: 14},
},
});
```

### `createStencil`

Stencils were updated to automatically add `box-sizing: border-box` to all stencils. If your stencil
did not add this style already, it may change the way `width` works for the component. Our intent is
to make all elements use border box layouts to make width calculations more predictable. This change
may change the way your component works if you use the `width` style property.

### `createStyles`

`createStyles` utility was updated to automatically add `box-sizing: border-box` to all styles. If
your styles functiion did not add this style already, it may change the way `width` works for the
component. Our intent is to make all elements use border box layouts to make width calculations more
predictable. This change may change the way your component works if you use the `width` style
property.

## Glossary

### Main
Expand Down
15 changes: 13 additions & 2 deletions modules/styling-transform/lib/utils/handleCreateStyles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,13 @@ export const handleCreateStyles: NodeTransformer = (node, context) => {
// https://ts-ast-viewer.com/#code/MYewdgzgLgBFCmBbADjAvDA3gKBjAZiCAFwwDkARgIYBOZ2AvkA
if (ts.isObjectLiteralExpression(arg)) {
const styleObj = parseObjectToStaticValue(arg, context);
return createStyleReplacementNode(node, styleObj, cssClassName, context);

return createStyleReplacementNode(
node,
{boxSizing: 'border-box', ...styleObj},
cssClassName,
context
);
}
// An Identifier is a variable. It could come from anywhere - imports, earlier
// assignments, etc. The easiest thing to do is to ask the TypeScript type checker what
Expand All @@ -74,7 +80,12 @@ export const handleCreateStyles: NodeTransformer = (node, context) => {
// The type must be a object
const styleObj = parseStyleObjFromType(type, context);

return createStyleReplacementNode(node, styleObj, cssClassName, context);
return createStyleReplacementNode(
node,
{boxSizing: 'border-box', ...styleObj},
cssClassName,
context
);
}
return arg;
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ export function getValueFromAliasedSymbol(
// If there is an aliased symbol and it is a variable declaration, try to resolve the
if (declaration && hasExpression(declaration)) {
if (declaration.initializer) {
declaration.initializer.getText(); //?
declaration.initializer.getText();
transform(declaration.initializer, context);

return getValueFromProcessedNodes(varName, context);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ describe('handleCreateStencil', () => {
})
`);

const result = transform(program, 'test.ts', withDefaultContext(program.getTypeChecker())); //?
const result = transform(program, 'test.ts', withDefaultContext(program.getTypeChecker()));

expect(result).toContain('styles: "box-sizing:border-box;--css-button-color:red;"');
});
Expand All @@ -166,7 +166,7 @@ describe('handleCreateStencil', () => {
})
`);

const result = transform(program, 'test.ts', withDefaultContext(program.getTypeChecker())); //?
const result = transform(program, 'test.ts', withDefaultContext(program.getTypeChecker()));

expect(result).toContain('styles: "box-sizing:border-box;--css-system-icon-size:1rem;"');
});
Expand Down
29 changes: 21 additions & 8 deletions modules/styling-transform/spec/utils/handleCreateStyles.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,17 @@ describe('createStyles', () => {
expect(result).toContain('createStyles(styles)');
});

it('should add box-sizing to styles', () => {
const program = createProgramFromSource(`
import {createStyles} from '@workday/canvas-kit-styling';
const styles = createStyles({})
`);

const result = transform(program, 'test.ts');

expect(result).toContain('styles: "box-sizing:border-box;"');
});

it('should parse simple objects with string values', () => {
const program = createProgramFromSource(`
import {createStyles, px2rem} from '@workday/canvas-kit-styling';
Expand All @@ -62,7 +73,7 @@ describe('createStyles', () => {

const result = transform(program, 'test.ts');

expect(result).toContain('font-size:0.75rem;');
expect(result).toContain('box-sizing:border-box;font-size:0.75rem;');
});

it('should parse simple objects with numeric values', () => {
Expand All @@ -76,7 +87,7 @@ describe('createStyles', () => {

const result = transform(program, 'test.ts');

expect(result).toContain('font-size:12px;');
expect(result).toContain('box-sizing:border-box;font-size:12px;');
});

it('should return an Emotion-optimized object', () => {
Expand All @@ -90,7 +101,9 @@ describe('createStyles', () => {

const result = transform(program, 'test.ts');

expect(result).toMatch(/{ name: "[a-z0-9]+", styles: "font-size:12px;" }/);
expect(result).toMatch(
/{ name: "[a-z0-9]+", styles: "box-sizing:border-box;font-size:12px;" }/
);
});

it('should handle nested selectors', () => {
Expand Down Expand Up @@ -602,7 +615,7 @@ describe('createStyles', () => {

expect(styles['test.css']).toContainEqual(
compileCSS(
'.css-my-component{background-color:red;}.css-my-component:hover{background:blue;}'
'.css-my-component{box-sizing:border-box;background-color:red;}.css-my-component:hover{background:blue;}'
)
);
});
Expand Down Expand Up @@ -632,7 +645,7 @@ describe('createStyles', () => {

const result = transform(program, 'test.ts');

expect(result).toContain('styles: "color:var(--cnvs-source-foo)');
expect(result).toContain('styles: "box-sizing:border-box;color:var(--cnvs-source-foo)');
});

it('should process variable keys from another file if processing is out of order', () => {
Expand Down Expand Up @@ -660,7 +673,7 @@ describe('createStyles', () => {

const result = transform(program, 'test.ts');

expect(result).toContain('styles: "--cnvs-source-foo:red;');
expect(result).toContain('styles: "box-sizing:border-box;--cnvs-source-foo:red;');
});

it('should process nested variable properties from another file if processing is out of order', () => {
Expand Down Expand Up @@ -688,7 +701,7 @@ describe('createStyles', () => {

const result = transform(program, 'test.ts');

expect(result).toContain('styles: "color:var(--cnvs-source-default-foo)');
expect(result).toContain('styles: "box-sizing:border-box;color:var(--cnvs-source-default-foo)');
});

it('should process nested variable keys from another file if processing is out of order', () => {
Expand Down Expand Up @@ -716,6 +729,6 @@ describe('createStyles', () => {

const result = transform(program, 'test.ts');

expect(result).toContain('styles: "--cnvs-source-default-foo:red;');
expect(result).toContain('styles: "box-sizing:border-box;--cnvs-source-default-foo:red;');
});
});
Loading
Loading