Skip to content

Add partial grid-area support, improve grid-row and grid-column#938

Merged
ai merged 1 commit intopostcss:masterfrom
alex7kom:master
Nov 8, 2017
Merged

Add partial grid-area support, improve grid-row and grid-column#938
ai merged 1 commit intopostcss:masterfrom
alex7kom:master

Conversation

@alex7kom
Copy link
Copy Markdown
Contributor

@alex7kom alex7kom commented Nov 8, 2017

Another take on #883

While the original issue suggested only translation of span syntax, this is an attempt to translate grid-area shorthand to -ms- properties as fully as possible. It is also applied to grid-row and grid-column shorthands.

Tests are failing:
1) 'removes unnecessary prefixes': presumably because it also should remove -ms- properties and currently it doesn't.
2) 'prevents doubling prefixes': it adds properties again.

Currently I'm trying to figure it out.

@ai
Copy link
Copy Markdown
Member

ai commented Nov 8, 2017

Tests fall. Seems like you fix can’t clean own prefixes. In this case, you can create separated test case just for grid-area and do not use this tests in removes unnecessary prefixes.

@alex7kom
Copy link
Copy Markdown
Contributor Author

alex7kom commented Nov 8, 2017

Done! I also have figured out 'doubling prefixes' check.
Now it fails on size limit, not sure what to do about it.

@ai ai assigned alex7kom and unassigned alex7kom Nov 8, 2017
@ai
Copy link
Copy Markdown
Member

ai commented Nov 8, 2017

@alex7kom just increase limit in package.json/size-limit :)

Comment thread lib/hacks/grid-area.js Outdated
const parser = require('postcss-value-parser');

const Declaration = require('../declaration');
const translateShorthand = require('./grid-shorthand');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you use it only in the single file, it will be better to put ./grid-shorthand.js content to grid-area.js.

Comment thread lib/hacks/grid-area.js Outdated
*/
insert(decl, prefix) {
if (prefix !== '-ms-') {
return;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think

return super.insert(decl, prefix)

is more future-proof :).

Comment thread lib/hacks/grid-area.js Outdated
const [rowStart, rowSpan] = translateShorthand(values, 0, 2);
const [columnStart, columnSpan] = translateShorthand(values, 1, 3);

rowStart && decl.cloneBefore({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if (rowStart) is more simple construction. It will be better to future developers.

Comment thread lib/hacks/grid-area.js
for (const i in decl.parent.nodes) {
if (decl.parent.nodes.hasOwnProperty(i)) {
const element = decl.parent.nodes[i];
if (element.prop === '-ms-grid-row') {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not future-proof. It is better to have this test in insert.

@alex7kom alex7kom changed the title Add partial grid-area shorthand translation support Add partial grid-area support, improve grid-row and grid-column Nov 8, 2017
@alex7kom
Copy link
Copy Markdown
Contributor Author

alex7kom commented Nov 8, 2017

Fixed everything noted, also added grid-row and grid-column translation.

@ai
Copy link
Copy Markdown
Member

ai commented Nov 8, 2017

Awesome!

@ai ai merged commit 1d3ef1c into postcss:master Nov 8, 2017
@ai ai added this to the 7.2 milestone Dec 3, 2017
@ai
Copy link
Copy Markdown
Member

ai commented Dec 3, 2017

Released in 7.2

Copy link
Copy Markdown

@Ayhanbey63500 Ayhanbey63500 left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants