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

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

Merged
merged 1 commit into from Nov 8, 2017

Conversation

Projects
None yet
2 participants
@alex7kom
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

ai commented Nov 8, 2017

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

const parser = require('postcss-value-parser');

const Declaration = require('../declaration');
const translateShorthand = require('./grid-shorthand');

This comment has been minimized.

@ai

ai Nov 8, 2017

Member

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

*/
insert(decl, prefix) {
if (prefix !== '-ms-') {
return;

This comment has been minimized.

@ai

ai Nov 8, 2017

Member

I think

return super.insert(decl, prefix)

is more future-proof :).

const [rowStart, rowSpan] = translateShorthand(values, 0, 2);
const [columnStart, columnSpan] = translateShorthand(values, 1, 3);

rowStart && decl.cloneBefore({

This comment has been minimized.

@ai

ai Nov 8, 2017

Member

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

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') {

This comment has been minimized.

@ai

ai Nov 8, 2017

Member

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

@alex7kom alex7kom changed the title from Add partial grid-area shorthand translation support to Add partial grid-area support, improve grid-row and grid-column Nov 8, 2017

@alex7kom

This comment has been minimized.

Contributor

alex7kom commented Nov 8, 2017

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

@ai

This comment has been minimized.

Member

ai commented Nov 8, 2017

Awesome!

@ai ai merged commit 1d3ef1c into postcss:master Nov 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ai ai added this to the 7.2 milestone Dec 3, 2017

@ai

This comment has been minimized.

Member

ai commented Dec 3, 2017

Released in 7.2

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