Skip to content

Conversation

@nebbles
Copy link
Contributor

@nebbles nebbles commented Jan 12, 2019

Proposed fix for #3447 where p5.Element.prototype.position of p5.dom does not style the element properly as called by e.g. style('position','fixed').

As seen in issue, the problem stems from p5.Element.prototype.style however, it seems that the design of this function is to delegate the assignment of transformations to their respective functions.

As such p5.Element.prototype.position was modified to be able to handle all cases, explicitly ensuring only valid values for CSS position are used.

This is my first PR so hopefully checked all the boxes 🤞

@limzykenneth
Copy link
Member

I think in the case of style('position','fixed') it is actually wrong to call p5.Element.prototype.position as they are different thing all together. style deals with css directly and in css position is the way the element is positioned while in p5 or rather p5.Element.prototype.position is the coordinates of the element. It shoud either pass through and set the css directly or a new helper function is written (which I don't think is necessary)

Also p5.Element.prototype.position is a public method so adding another overload like this could potentially cause problem unless documented.

@nebbles
Copy link
Contributor Author

nebbles commented Jan 14, 2019

As referred to in #3447, it does seem to be strange behaviour for style:

p5.Element.prototype.style = function(prop, val) {
    var self = this;

    // ...

    if (typeof val === 'undefined') {
        // ...
    } else {
      if (prop === 'rotate' || prop === 'translate' || prop === 'position') {
        var trans = Array.prototype.shift.apply(arguments);
        var f = this[trans] || this['_' + trans];
        f.apply(this, arguments);
      } else {
        // ...
      }
    }
    return this;
  };

and so its really the application of position style here that needs the fix in that case.

Happy to make the alteration here instead? Would certainly cause less confusion if position is a public method exclusively for setting absolute coordinates.

@Ajayneethikannan
Copy link
Contributor

Ajayneethikannan commented Feb 14, 2019

@nebbles @limzykenneth , I agree that the position function should not be overloaded , but in my humble opinion, it would be nice to see if it supported other position attributes instead of only being restricted to "absolute"

The use case of other properties may arise when the canvas may be present inside another div,
this issue can be considered as an example ,
processing/p5.js-website#394
where the parent div is "relative" , preventing us to set position with respect to window.
My suggestion is an optional third parameter , so that we can call the function as:

some_elt.position(10, 10, ABSOLUTE);
or

some_elt.position.(10, 10, FIXED); (always relative to the current viewport)

with default value being absolute.

Waiting for your thoughts about it ,
Thank you!

@limzykenneth
Copy link
Member

@Ajayneethikannan I think the proposal of something like some_elt.position(10, 10, ABSOLUTE); can be opened as a separate feature request issue for discussions.

@Ajayneethikannan
Copy link
Contributor

Ajayneethikannan commented Mar 5, 2019

I have opened a new issue for discussion at #3570 .
Thank you @limzykenneth !

@lmccart
Copy link
Member

lmccart commented Apr 8, 2019

thanks @nebbles! we've now addressed #3447 by removing the support for special arg "position" from style, so it can be handled directly by position() function. we can continue discussion of the arguments for that function over in #3570.

@lmccart lmccart closed this Apr 8, 2019
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.

5 participants