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

maping through nested rules #2935

Closed
wants to merge 2 commits into from
Closed

maping through nested rules #2935

wants to merge 2 commits into from

Conversation

makbari
Copy link

@makbari makbari commented Aug 3, 2020

This is integration of the nested rules. When nested at rules is parsed from grapesjs-parset-postcss, grapes js resolve them.
an example is also provided in the postcss repo.

Copy link
Member

@artf artf left a comment

Choose a reason for hiding this comment

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

Can you please explain the point of this PR? What are you trying to solve?

@@ -107,11 +107,15 @@ export default Backbone.Model.extend({
const selectorStrNoAdd = rule.selectorsToString({ skipAdd: 1 });
const selectorsAdd = rule.get('selectorsAdd');
const singleAtRule = rule.get('singleAtRule');
let isGjsRule = false;
Copy link
Member

Choose a reason for hiding this comment

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

what is the point of this variable? Seems like you don't use it...

@@ -15,7 +15,7 @@ const atRules = {
};
const atRuleKeys = keys(atRules);
const singleAtRules = ['5', '6', '11', '15'];
const singleAtRulesNames = ['font-face', 'page', 'counter-style', 'viewport'];
const singleAtRulesNames = ['font-face', 'counter-style', 'viewport'];
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the page?

@@ -121,8 +125,10 @@ export default Backbone.Model.extend({
}
});

if ((selectorStrNoAdd && found) || selectorsAdd || singleAtRule) {
const block = rule.getDeclaration();
if ((selectorStrNoAdd && found) || selectorsAdd || singleAtRule || rule.attributes.style) {
Copy link
Member

Choose a reason for hiding this comment

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

rule.attributes.style is an object, it's a truthy value, so the condition is pointless

if ((selectorStrNoAdd && found) || selectorsAdd || singleAtRule || rule.attributes.style) {
const entries = Object.entries(rule.attributes.style);
const styleStr = entries.length > 0 ? entries.map(([k, v]) => `${k}:${v}`).join(';') + ';' : '';
const block = rule.getDeclaration().length > 0 ? rule.getDeclaration() : styleStr;
Copy link
Member

Choose a reason for hiding this comment

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

You're calling rule.getDeclaration() twice and honestly, I don't understand the point of this change

@makbari
Copy link
Author

makbari commented Aug 31, 2020

Thanks for the review. I will make some correction and more updates.
There problem I am trying to solve is that, page at rule, can have nested at rule.
The example is also in GrapesJS/parser-postcss#1

@artf artf closed this Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants