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

[Bug]: custom style property is not working when toRequire attribute is set #2657

Closed
Podvodila opened this issue Mar 16, 2020 · 6 comments
Closed

Comments

@Podvodila
Copy link

  1. Are you using the latest release (older versions are NOT supported)?
    Yes
  2. What is the expected behavior?
    Custom style property applying to component
  3. What happens instead?
    Custom style property is not working
  4. Steps to reproduce
  • select Box component
  • try to change Box Width style property (under Custom tab)
    image

When I add custom style property(flex) with toRequire: 1 parameter then custom style is not working. When I mark toRequire as 0 then style property work as expected.

JSFiddle with toRequire: 1 - https://jsfiddle.net/rjw7Lq93/1/
JSFiddle with toRequire: 0 - https://jsfiddle.net/rjw7Lq93/2/

@Podvodila
Copy link
Author

Podvodila commented Mar 18, 2020

I managed to hotfix it by extending isTargetStylable method inside PropertyView.js - https://github.com/artf/grapesjs/blob/85b7b82103e12fb01a3700b1002a57a7b60ccaf1/src/style_manager/view/PropertyView.js#L504-510

I replaced this

if (toRequire) {
stylable =
!target ||
(stylableReq &&
(stylableReq.indexOf(id) >= 0 || stylableReq.indexOf(property) >= 0));
}

with this

if (toRequire) {
stylable =
!target ||
trg.cid === this.getTarget().cid ||
(stylableReq &&
(stylableReq.indexOf(id) >= 0 || stylableReq.indexOf(property) >= 0));
}

It seems that the wrong target parameter comes inside the method on component style change

JSFiddle with extended PropertyView - https://jsfiddle.net/6zf03t9w/

@Podvodila
Copy link
Author

Offtop
I don't want to create another topic, just want to say that in the same method(isTargetStylable) that would be better to replace searching for property with id(as I understand for the built-in styles, it is the same as the property) inside unstylable(as well as stylable) check

if (_.isArray(unstylable)) {
stylable = unstylable.indexOf(property) < 0;
}

->

if (_.isArray(unstylable)) {
stylable = unstylable.indexOf(id) < 0;
}

Since I had a problem with displaying a custom style for a component of the same property as the built-in one. (in my application, the built-in style should be displayed for some components, and custom for others)

@artf
Copy link
Member

artf commented Apr 3, 2020

would be better to replace searching for property with id(as I understand for the built-in styles, it is the same as the property) inside unstylable(as well as stylable) check

Yeah, totally correct!

Honestly, I use toRequire option and never seen this kind of issue so I'm not sure why this happening. Would you mind to create a PR with your fixes (maybe I'll try to investigate better)?

@Smritibajaj
Copy link

Smritibajaj commented Sep 1, 2020

@artf
Any update on this bug?

@CaseJnr
Copy link

CaseJnr commented Sep 6, 2020

@artf Same problem here. I rolled back to 0.15.8 to fix the issue.

@artf
Copy link
Member

artf commented Sep 12, 2020

Will be fixed in the next release, for now, you can avoid the issue by using this option on init

selectorManager: { componentFirst: true },

@Podvodila In this way, you don't need to toggle off all selectors as the component will be selected by default

@artf artf added this to To do in Release v0.16.27 via automation Sep 12, 2020
@artf artf closed this as completed in 9c151bd Sep 14, 2020
Release v0.16.27 automation moved this from To do to Done Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

4 participants