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

Correctly update the style attr #53

Closed
wants to merge 1 commit into from
Closed

Correctly update the style attr #53

wants to merge 1 commit into from

Conversation

psiska
Copy link

@psiska psiska commented May 31, 2012

Hi,
I have made a small change in javascript code to correctly handle update of style property of reactive element.

I would be glad if this fix can be incorporated.

Thanks

@nafg
Copy link
Owner

nafg commented Jun 11, 2012

Hmm, good point. But what about, for example, background-color to backgroundColor?
Does it work if instead you write PropertyVar("style.cssText", "style") ?

@psiska
Copy link
Author

psiska commented Jun 14, 2012

I don't think so. This will invoke function updateProperty function which will do element['style.cssText'] = 'style';
which does not work.

I think there are several ways to solve the issue.

  • use css styles and updates styles on element - (and close this pull request)
  • provide some kind of mapping from css property name to dom property name (e.g. background-color to backgroundColor)
  • merge the pull request and the user of this feature has to be aware of correct dom property name.

What do you think?

@nafg
Copy link
Owner

nafg commented Jun 14, 2012

On Thu, Jun 14, 2012 at 1:54 PM, Peter Siska <
reply@reply.github.com

wrote:

I don't think so. This will invoke function updateProperty function which
will do element['style.cssText'] = 'style';
which does not work.

Good point :)

I think there are several ways to solve the issue.

  • use css styles and updates styles on element - (and close this pull
    request)
  • provide some kind of mapping from css property name to dom property name
    (e.g. background-color to backgroundColor)
  • merge the pull request and the user of this feature has to be aware of
    correct dom property name.

What do you think?

Sorry, not clear what you meant by #1 and #3. Can you elaborate?


Reply to this email directly or view it on GitHub:
#53 (comment)

@psiska
Copy link
Author

psiska commented Jun 22, 2012

Oh ok,

    • consider not changing the updateProperty js function.
      So if user would like to change visual appearance of element, he has to provide css class (with that change). Than using standard PropertyVar("className") set the css class.
  1. solution would be to merge my proposed change. User should be aware of the discrepancy between css and dom attribute names.

Hope I made it more clear.

@nafg
Copy link
Owner

nafg commented Jun 24, 2012

Sorry to be dense, but now I'm not sure of the difference between #2 and #3.

Anyway I think the generally recommended pattern for web developers is
probably to toggle the class, not the style (#1 if I understand correctly).
However I'm sure there are cases where that's not practical, so we should
provide an ability to change the style directly.

I have a different approach to suggest though --- JsStub style. Have you
seen it?

On Fri, Jun 22, 2012 at 8:48 AM, Peter Siska <
reply@reply.github.com

wrote:

Oh ok,

#1 - consider not changing the updateProperty js function.
So if user would like to change visual appearance of element, he has to
provide css class (with that change). Than using standard
PropertyVar("className") set the css class.

#3 solution would be to merge my proposed change. The discrepancy between
css and dom attribute names should kept in some sort of map, so user will
use only css attribute names.
These names will reactive automagically convert to correct dom attribute
names.

Hope I made it more clear.


Reply to this email directly or view it on GitHub:
#53 (comment)

@nafg
Copy link
Owner

nafg commented May 28, 2014

I think we need a solution general enough to help with PropertyVar.toggleClass as well.

@psiska psiska closed this Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants