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

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

Conversation

Dobby85
Copy link
Contributor

@Dobby85 Dobby85 commented May 17, 2024

Current behavior

The current behavior describe below can be tested on this JSFiddle : https://jsfiddle.net/728urdh3/

Context

I add a custom component (sectionBlock) that have styles by the section tag name and the private selector ed-layout-div.

section {
  width: 940px;
  min-height: 50px;
  border: 1px dashed black;
}

.ed-layout-div {
  margin-top: 20px;
  margin-bottom: 30px;          
}

Step to reproduce

When you drop this component in the canvas and check it's style.

  • You do not see width and min-height properties set on the section.
  • You see the margin-top and the margin-bottom apply on the hidden selectors.

Screenshot 2024-05-17 at 08 25 43

Now when you add a class to the component :

  • You do not see anymore the margin-top and the margin-bottom apply on the hidden selectors.

Screenshot 2024-05-17 at 08 27 20

Expected and new behavior

Context

The context is the same than the previous one

Step to reproduce

When you drop this component in the canvas and check it's style.

  • You see width and min-height properties set on the section as parent rules
  • You see margin-top and margin-bottom apply on the hidden selectors as parent rules

Screenshot 2024-05-17 at 08 28 16

Now when you add a class to the component :

  • You still see width and min-height properties set on the section as parent rules
  • You still see margin-top and margin-bottom apply on the hidden selectors as parent rules

Screenshot 2024-05-17 at 08 28 31

Why do I do that?

I think it's more comfortable to always see the style that is apply on the component to better understand why it is like it is. You still can override style but you always know which rule are applied to the component.

All tests passed and I add 3 new tests to test style apply on tag name and return style with multiple classes.

Do not hesitate if you need more information or if I have to update/fix something !

Have a great day!

Dobby85 and others added 3 commits May 16, 2024 18:49
Fix bug with private selectors that stay invisible in style manager when another class was applied.
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.

Thank you @Dobby85 that's a nice idea and overall a good PR.
I'd just ask you to review my comments and keep performance in mind here to avoid falling into similar issues.

src/style_manager/index.ts Outdated Show resolved Hide resolved
.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 !

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.

src/style_manager/index.ts Show resolved Hide resolved
src/style_manager/index.ts Outdated Show resolved Hide resolved
test/specs/style_manager/index.ts Show resolved Hide resolved
@Dobby85
Copy link
Contributor Author

Dobby85 commented May 29, 2024

@artf I don't know who is supposed to resolve conversation. I fix everything I could based on your comment, I let you review changes and resolve conversation if it sounds good to you ☺️

@Dobby85 Dobby85 requested a review from artf May 29, 2024 09:19
artf and others added 3 commits June 3, 2024 17:45
…rents-cssrule' of github.com:Dobby85/grapesjs into handle-style-set-to-tag-name-and-multiple-classes-in-parents-cssrule
@Dobby85
Copy link
Contributor Author

Dobby85 commented Jun 7, 2024

All your comments should be resolve @artf. Tell me if it's not! Else I think everything is ready for merge 😁

@artf artf merged commit 3f5056d into GrapesJS:dev Jun 11, 2024
2 checks passed
@artf
Copy link
Member

artf commented Jun 11, 2024

Nice, thanks @Dobby85

@chen-can
Copy link

您的所有意见都应该得到解决@artf。如果不是的话请告诉我!不然我想一切都已经准备好进行合并了😁

<style> .mw-package-price, .mw-package-price span.price { color: red; } .mw-package-price span { color: #4d4d4d; } </style>
  <p class="mw-package-price">
    <span class="regular-price">$98.00</span>
    <span class="price" id="prod-1-price">$49.00</span>
  </p>
  fail 

@chen-can
Copy link

Version 0.17.29 works and matches correctly, but the newer version does not. I need to use the newer version because grapesjs-style-bg requires a better version

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.

3 participants