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

Show style applied on tag (p, span, div) and private selectors as parent rules instead of hiding them #5890

Merged
merged 7 commits into from
Jun 11, 2024
Merged

Show style applied on tag (p, span, div) and private selectors as parent rules instead of hiding them #5890

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
52ba783
GrapesJS can now returns multiple CSS rules class as parents and CSS …
Dobby85 May 16, 2024
12b0452
Take last version of grape.
Dobby85 May 17, 2024
848fbca
Merge branch 'dev' into handle-style-set-to-tag-name-and-multiple-cla…
Dobby85 May 21, 2024
9f6289c
Fix reviews;
Dobby85 May 29, 2024
26ee377
Merge branch 'dev' into handle-style-set-to-tag-name-and-multiple-cla…
artf Jun 3, 2024
3ee8d71
Simplify code to work only with single tag name;
Dobby85 Jun 7, 2024
d2adb8b
Merge branch 'handle-style-set-to-tag-name-and-multiple-classes-in-pa…
Dobby85 Jun 7, 2024
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
52 changes: 45 additions & 7 deletions src/style_manager/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,27 +597,65 @@ export default class StyleManager extends ItemManagerModule<
const cmp = target.toHTML ? target : target.getComponent();
const optsSel = { array: true } as const;
let cmpRules: CssRule[] = [];
let tagNameRules: CssRule[] = [];
let invisibleRules: CssRule[] = [];
let otherRules: CssRule[] = [];
let rules: CssRule[] = [];

const rulesBySelectors = (values: string[]) => {
return !values.length
? []
: cssC.getRules().filter(rule => {
const rSels = rule.getSelectors().map(s => s.getFullName());
return !!rSels.length && rSels.every(rSel => values.indexOf(rSel) >= 0);
return cssC.getRules().filter(rule => {
const rSels = rule.getSelectors().map(s => s.getFullName());

// rSels is equal to 0 when rule selectors contain tagName like : p {}, .logo path {}, ul li {}
if (rSels.length === 0) {
return false;
}

return rSels.every(rSel => values.indexOf(rSel) >= 0);
});
};

const rulesByTagName = (tagName: string) => {
return cssC.getRules().filter(rule => {
const ruleTags = rule
.selectorsToString()
.split(' ')
.filter(item => {
return item.startsWith('.') === false && item.startsWith('#') === false;
Copy link
Member

Choose a reason for hiding this comment

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

why not to simply check rule.selectorsToString() === tagName?
The logic here will take wrong rules, eg. .something DIV .something-else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot just check rule.selectorsToString() === tagName because tagName only include the tag name like div, section, li... and not all the path like rule.selectorsToString() can have (.something .something-else div).

In fact, it's a problem for this case: .something DIV .something-else. So I add a check to be sure the tagName is the last element in the rule.selectorsToString().

I know this function is not perfect, it still contains this caveat:

<section class="cls1">
  <div>Text</div>
</section>
.cls1 div {
  padding: 10px;
}

In the previous case,.cls1 div will be return as a rule for both div because I don't how to check a component is a child or not of a component having .cls1. Or if we can check that I think we will add a lot of loops...

I don't know what is the best things to do:

1 - Know the caveat and let it like this
2 - Try to fix it and add a lot of checks and loops

In every case, I fix the following case .something DIV .something-else and add a test on it :)

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to keep it simple for now, only selectors with one and single tagName are accepted, eg. div is ok, .cls1 div is not. So let's just do rule.selectorsToString() === tagName.

You're not considering a lot of CSS selectors, like in your case .cls2 div will be taken as valid, also .cls2 div div... it's better to skip those rules instead of showing the wrong ones.
We won't be able to be precise with the current approach anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did what you say. So it's only working for single tag name. It simplify the code too so it's a good thing !

Copy link

Choose a reason for hiding this comment

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

The 0.17 version is supported.

});

if (ruleTags.length === 0) {
return false;
}

const lastTags = ruleTags[ruleTags.length - 1];

return lastTags === tagName;
});
};

// Componente related rule
if (cmp) {
cmpRules = cssC.getRules(`#${cmp.getId()}`);
tagNameRules = rulesByTagName(cmp.get('tagName'));
otherRules = sel ? rulesBySelectors(sel.getSelectors().getFullName(optsSel)) : [];
rules = otherRules.concat(cmpRules);
rules = otherRules.concat(tagNameRules).concat(cmpRules);
} else {
cmpRules = sel ? cssC.getRules(`#${sel.getId()}`) : [];
tagNameRules = rulesByTagName(sel?.get('tagName') || '');
// Get rules set on active and visible selectors
otherRules = rulesBySelectors(target.getSelectors().getFullName(optsSel));
rules = cmpRules.concat(otherRules);
// Get rules set on invisible selectors like private one
const allCmpClasses = sel?.getSelectors().getFullName(optsSel) || [];
const invisibleSel = allCmpClasses.filter(
(item: string) =>
target
.getSelectors()
.getFullName(optsSel)
.findIndex(sel => sel === item) === -1
);
invisibleRules = rulesBySelectors(invisibleSel);
rules = tagNameRules.concat(cmpRules).concat(invisibleRules).concat(otherRules);
}

const all = rules
Expand Down
38 changes: 38 additions & 0 deletions test/specs/style_manager/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,44 @@ describe('StyleManager', () => {
expect(obj.getSelectedParents()).toEqual([rule1]);
});

test('With multiple classes, should both be in parents list', () => {
const cmp = domc.addComponent('<div class="cls cls2"></div>');
const [rule1, rule2] = cssc.addRules(`
.cls { color: red; }
.cls2 { color: blue; }
`);
em.setSelected(cmp);
obj.__upSel();
expect(obj.getSelected()).not.toBe(rule1);
expect(obj.getSelected()).not.toBe(rule2);
expect(obj.getSelectedParents()).toEqual([rule2, rule1]);
});

test('With tagName + class, class first', () => {
const cmp = domc.addComponent('<div class="cls" id="id-test"></div>');
const [rule1, rule2] = cssc.addRules(`
.cls { color: red; }
div { color: yellow; }
`);
em.setSelected(cmp);
obj.__upSel();
expect(obj.getSelected()).toBe(rule1);
expect(obj.getSelectedParents()).toEqual([rule2]);
});

test('With tagName + ID + class, class first, ID second', () => {
const cmp = domc.addComponent('<div class="cls" id="id-test"></div>');
const [rule1, rule2, rule3] = cssc.addRules(`
.cls { color: red; }
div { color: yellow; }
#id-test { color: blue; }
`);
em.setSelected(cmp);
obj.__upSel();
expect(obj.getSelected()).toBe(rule1);
expect(obj.getSelectedParents()).toEqual([rule3, rule2]);
});

test('With ID + class, multiple devices', () => {
sm.setComponentFirst(true);
const cmp = domc.addComponent('<div class="cls" id="id-test"></div>');
Expand Down
Loading