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.

Sign up for GitHub

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

fix: simplify style logic in template compiler #314

Merged
merged 5 commits into from
Jun 12, 2018
Merged

fix: simplify style logic in template compiler #314

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0b27792
fix: simplify style logic in template compiler
pmdartus May 17, 2018
33236d3
fix: remove unused API to fix linting error
pmdartus May 17, 2018
48f9cf4
fix: use camelCase instead of kebab-case for css properties
pmdartus May 18, 2018
edf8b41
Merge branch 'master' into pmdartus/fix-invalid-style-parsing
pmdartus May 30, 2018
5d37713
docs: add comments to clarify the style changes
pmdartus May 30, 2018
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
2 changes: 1 addition & 1 deletion packages/lwc-engine/src/3rdparty/snabbdom/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export interface VNodeData {
props?: Props;
attrs?: Attrs;
class?: Classes;
style?: VNodeStyle;
style?: VNodeStyle | string;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should mark this as a difference since this file is copied and modified from snabbdom, so we know when copying the new one.

Copy link
Contributor

Choose a reason for hiding this comment

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

... @pmdartus

Copy link
Contributor

Choose a reason for hiding this comment

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

additionally, we can expand this type in LWC only.

Copy link
Member Author

Choose a reason for hiding this comment

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

We will fix this in an upcoming PR when removing the engine registry.

// dataset?: Dataset;
on?: On;
// hero?: Hero;
Expand Down
23 changes: 7 additions & 16 deletions packages/lwc-engine/src/framework/modules/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,17 @@ import assert from "../assert";
import {
isString,
isUndefined,
StringCharCodeAt,
} from '../language';
import { EmptyObject } from '../utils';
import { VNode, Module } from "../../3rdparty/snabbdom/types";
import { VNode, Module, VNodeStyle } from "../../3rdparty/snabbdom/types";
import { removeAttribute } from '../dom';

const DashCharCode = 45;

function updateStyle(oldVnode: VNode, vnode: VNode) {
const { data: { style: newStyle } } = vnode;
const { style: newStyle } = vnode.data;
if (isUndefined(newStyle)) {
return;
}
let { data: { style: oldStyle } } = oldVnode;
let { style: oldStyle } = oldVnode.data;
if (oldStyle === newStyle) {
return;
}
Expand All @@ -27,13 +24,13 @@ function updateStyle(oldVnode: VNode, vnode: VNode) {
let name: string;
const elm = (vnode.elm as HTMLElement);
const { style } = elm;
if (isUndefined(newStyle) || newStyle as any === '') {
if (isUndefined(newStyle) || newStyle === '') {
removeAttribute.call(elm, 'style');
} else if (isString(newStyle)) {
style.cssText = newStyle;
} else {
if (!isUndefined(oldStyle)) {
for (name in oldStyle) {
for (name in oldStyle as VNodeStyle) {
if (!(name in newStyle)) {
style.removeProperty(name);
}
Expand All @@ -44,14 +41,8 @@ function updateStyle(oldVnode: VNode, vnode: VNode) {

for (name in newStyle) {
const cur = newStyle[name];
Copy link
Member Author

Choose a reason for hiding this comment

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

Add comment here.

if (cur !== (oldStyle as any)[name]) {
if (StringCharCodeAt.call(name, 0) === DashCharCode && StringCharCodeAt.call(name, 1) === DashCharCode) {
// if the name is prefixed with --, it will be considered a variable, and setProperty() is needed
style.setProperty(name, cur);
} else {
// @ts-ignore
style[name] = cur;
}
if (cur !== (oldStyle as VNodeStyle)[name]) {
style.setProperty(name, cur);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is code copied from snabbdom (mostly). I wonder why did they use style[name] = cur instead of setProperty. was it because of the camelCase?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes the setProperty accepts the kebab-case version of it. While the direct set on the object accepts the camelCase.

I think they want to privilege the camelCase approach because of the developer ergonomic:

// camelCase
h('span', {
  style: {
	border: '1px solid #bada55', 
	fontWeight: 'bold'
  }
}, 'Say my name, and every colour illuminates');

// kebab-case
h('span', {
  style: {
	border: '1px solid #bada55', 
	'font-weight': 'bold'
  }
}, 'Say my name, and every colour illuminates');

But because we don't allow objects for styles, we don't have to deal with the camelCase.

}
}
}
Expand Down
2 changes: 0 additions & 2 deletions packages/lwc-template-compiler/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@
"babel-traverse": "^6.26.0",
"babel-types": "^6.26.0",
"babylon": "^6.17.0",
"camelcase": "^4.1.0",
"css-parse": "^2.0.0",
"decamelize": "^1.2.0",
"he": "^1.1.1",
"parse5-with-errors": "^4.0.1"
Expand Down
7 changes: 2 additions & 5 deletions packages/lwc-template-compiler/src/__tests__/fixtures/base/style-static/expected.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,9 @@ export default function tmpl($api, $cmp, $slotset, $ctx) {
'section',
{
styleMap: {
fontSize: '12px',
'font-size': '12px',
color: 'red',
marginLeft: '5px',
marginRight: '5px',
marginTop: '10px',
marginBottom: '10px'
margin: '10px 5px 10px'
},
key: 1
},
Expand Down
7 changes: 2 additions & 5 deletions packages/lwc-template-compiler/src/__tests__/parser.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,9 @@ describe('class and style', () => {
<section style="font-size: 12px; color: red; margin: 10px 5px 10px"></section>
</template>`);
expect(root.children[0].styleMap).toEqual({
fontSize: '12px',
['font-size']: '12px',
color: 'red',
marginLeft: '5px',
marginRight: '5px',
marginTop: '10px',
marginBottom: '10px',
margin: '10px 5px 10px'
});
});

Expand Down
68 changes: 68 additions & 0 deletions packages/lwc-template-compiler/src/parser/__tests__/style.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { parseStyleText, parseClassNames } from '../style';

describe('parseStyleText', () => {
it('should parse simple style text', () => {
const res = parseStyleText('color: blue');
expect(res).toEqual({ color: 'blue' });
});

it('should parse simple style text with trailing coma', () => {
const res = parseStyleText('color: blue;');
expect(res).toEqual({
color: 'blue'
});
});

it('should parse simple style with multiple values', () => {
const res = parseStyleText('box-shadow: 10px 5px 5px black;');
expect(res).toEqual({
['box-shadow']: '10px 5px 5px black'
});
});

it('should parse multiple declaration', () => {
const res = parseStyleText(`font-size: 12px;background: blue; color:red ;`);
expect(res).toEqual({
['font-size']: '12px',
background: 'blue',
color: 'red'
});
});

it('should parse css functions', () => {
const res = parseStyleText(`background-color:rgba(255,0,0,0.3)`);
expect(res).toEqual({
['background-color']: 'rgba(255,0,0,0.3)'
});
});

it('should support base 64 encoded strings', () => {
const image = 'url("data:image/webp;base64,AAAAAAAAAAA")';
const res = parseStyleText(`background: ${image}`);
expect(res).toEqual({
background: image
});
});
});

describe('parseClassNames', () => {
it('should support a single class', () => {
const res = parseClassNames('foo');
expect(res).toEqual({ foo: true });
});

it('should support simple class list', () => {
const res = parseClassNames('foo bar');
expect(res).toEqual({ foo: true, bar: true });
});

it('should support simple class list with trailing spaces', () => {
const res = parseClassNames(' foo bar ');
expect(res).toEqual({ foo: true, bar: true });
});

it('should support simple class list multiple spaces', () => {
const res = parseClassNames('foo bar');
expect(res).toEqual({ foo: true, bar: true });
});
});
4 changes: 2 additions & 2 deletions packages/lwc-template-compiler/src/parser/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
} from './expression';

import {
parseStyle,
parseStyleText,
parseClassNames,
} from './style';

Expand Down Expand Up @@ -266,7 +266,7 @@ export default function parse(source: string, state: State): {
if (styleAttribute.type === IRAttributeType.Expression) {
element.style = styleAttribute.value;
} else if (styleAttribute.type === IRAttributeType.String) {
element.styleMap = parseStyle(styleAttribute.value);
element.styleMap = parseStyleText(styleAttribute.value);
}
}
}
Expand Down
189 changes: 28 additions & 161 deletions packages/lwc-template-compiler/src/parser/style.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,178 +1,45 @@
import * as parseCSS from 'css-parse';
import * as toCamelCase from 'camelcase';

const NOT_SUPPORTED = ['display'];

const DIRECTIONS = ['top', 'right', 'bottom', 'left'];
const CHANGE_ARR = ['margin', 'padding', 'border-width', 'border-radius'];

const NUMBERIZE = ['width', 'height', 'font-size', 'line-height'].concat(DIRECTIONS);
DIRECTIONS.forEach((dir) => {
NUMBERIZE.push(`border-${dir}-width`);
CHANGE_ARR.forEach((prop) => {
NUMBERIZE.push(`${prop}-${dir}`);
});
});

// Special properties and shorthands that need to be broken down separately
const SPECIAL_PROPS: { [name: string]: { regex: RegExp, map: { [key: number]: string | null } } } = {};
['border', 'border-top', 'border-right', 'border-bottom', 'border-left'].forEach((name) => {
SPECIAL_PROPS[name] = {
/* uncomment to remove `px` */
// regex: /^\s*([0-9]+)(px)?\s+(solid|dotted|dashed)?\s*([a-z0-9#,\(\)\.\s]+)\s*$/i,
regex: /^\s*([0-9]+px?)\s+(solid|dotted|dashed)?\s*([a-z0-9#,\(\)\.\s]+)\s*$/i,
map: {
1: `${name}-width`,
3: name === 'border' ? `${name}-style` : null,
4: `${name}-color`,
},
};
});

// Map of properties that when expanded use different directions than the default Top,Right,Bottom,Left.
const DIRECTION_MAPS: { [name: string]: { [direction: string]: string } } = {
'border-radius': {
Top: 'top-left',
Right: 'top-right',
Bottom: 'bottom-right',
Left: 'bottom-left',
},
};

function clean(value: string): string {
return value.replace(/\r?\n|\r/g, '');
export interface StyleMap {
[name: string]: string;
}

// Convert the shorthand property to the individual directions, handles edge cases.
// i.e. border-width and border-radius
function directionToPropertyName(property: string, direction: string): string {
const names = property.split('-');
names.splice(1, 0, DIRECTION_MAPS[property] ? DIRECTION_MAPS[property][direction] : direction);
return toCamelCase(names.join('-'));
export interface ClassMap {
[name: string]: true;
}

// FIXME: This function is crap and need to be better tested
function parse(styleString: string): any {
const stylesheetString = `body { ${styleString} }`;

const { stylesheet } = parseCSS(clean(stylesheetString));

const JSONResult: any = {};

for (const rule of stylesheet.rules) {
if (rule.type !== 'rule') {
continue;
}

for (let selector of rule.selectors) {
selector = selector.replace(/\.|#/g, '');
const styles = (JSONResult[selector] = JSONResult[selector] || {});

for (const declaration of rule.declarations) {
if (declaration.type !== 'declaration') {
continue;
}

const { value, property } = declaration;
const DECLARATION_DELIMITER = /;(?![^(]*\))/g;
const PROPERTY_DELIMITER = /:(.+)/;

if (SPECIAL_PROPS[property]) {
const special: any = SPECIAL_PROPS[property];
const matches = special.regex.exec(value as string);
if (matches) {
if (typeof special.map === 'function') {
special.map(matches, styles, rule.declarations);
} else {
for (const key in special.map) {
if (matches[key] && special.map[key]) {
rule.declarations.push({
position: declaration.position,
parent: rule,
property: special.map[key] || '',
value: matches[key],
type: 'declaration',
});
}
}
}
continue;
}
}
// Borrowed from Vue template compiler:
// https://github.com/vuejs/vue/blob/531371b818b0e31a989a06df43789728f23dc4e8/src/platforms/web/util/style.js#L5-L16
export function parseStyleText(cssText: string): StyleMap {
const styleMap: StyleMap = {};

if (NOT_SUPPORTED.includes(property)) {
continue;
}
const declarations = cssText.split(DECLARATION_DELIMITER);
for (const declaration of declarations) {
if (declaration) {
const [prop, value] = declaration.split(PROPERTY_DELIMITER);

if (NUMBERIZE.includes(property)) {
// uncomment to remove `px`
// value = value.replace(/px|\s*/g, '');
styles[toCamelCase(property)] = /*parseFloat(value);*/ value; /* uncomment to remove `px` */
} else if (CHANGE_ARR.includes(property)) {
const values = (value as string)/*.replace(/px/g, '')*/.split(/[\s,]+/);

/* uncomment to remove `px` */
// values.forEach((value, index, arr) => {
// arr[index] = parseInt(value);
// });

const length = values.length;

if (length === 1) {
styles[toCamelCase(property)] = values[0];
}

if (length === 2) {
for (const prop of ['Top', 'Bottom']) {
styles[directionToPropertyName(property, prop)] = values[0];
}

for (const prop of ['Left', 'Right']) {
styles[directionToPropertyName(property, prop)] = values[1];
}
}

if (length === 3) {

for (const prop of ['Left', 'Right']) {
styles[directionToPropertyName(property, prop)] = values[1];
}

styles[directionToPropertyName(property, 'Top')] = values[0];
styles[directionToPropertyName(property, 'Bottom')] = values[2];
}

if (length === 4) {
['Top', 'Right', 'Bottom', 'Left'].forEach((prop, index) => {
styles[directionToPropertyName(property, prop)] = values[index];
});
}
} else {
const shouldParseFloat = (typeof declaration.value === 'number' && !isNaN(declaration.value))
&& property !== 'font-weight';
if (shouldParseFloat) {
declaration.value = parseFloat(declaration.value as string);
}

styles[toCamelCase(property)] = declaration.value;
}
if (prop !== undefined && value !== undefined) {
styleMap[prop.trim()] = value.trim();
}
}
}

return JSONResult.body;
return styleMap;
}

export function parseStyle(style: string): { [name: string]: string } {
return parse(style);
}
const CLASSNAME_DELIMITER = /\s+/;

export function parseClassNames(classNames: string): ClassMap {
const classMap: ClassMap = {};

export function parseClassNames(classNames: string): { [name: string]: true } {
const splitted = classNames.trim()
.split(/\s+/)
.filter((className) => className.length);
const classList = classNames.split(CLASSNAME_DELIMITER);
for (const className of classList) {
const normalizedClassName = className.trim();

const classObj: { [name: string]: true } = {};
for (const className of splitted) {
classObj[className] = true;
if (normalizedClassName.length > 0) {
classMap[className] = true;
}
}
return classObj;
return classMap;
}
Loading