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

Dynamic positioning #30

Merged
merged 4 commits into from
Nov 13, 2016
Merged

Dynamic positioning #30

merged 4 commits into from
Nov 13, 2016

Conversation

tarekis
Copy link
Contributor

@tarekis tarekis commented Nov 8, 2016

Adds dynamic positioning via any valid DOM2 Element, as discussed in #28.

as of 28#issuecomment-258837864
as of #07e57b4e9feba18d586cde124e7a1d76a0841aab
@tarekis
Copy link
Contributor Author

tarekis commented Nov 8, 2016

gh-pages implementation coming soon too, just messed some branching up, gonna fix that if that PR makes it.

@SpaceK33z
Copy link
Contributor

Great work. I'll review this properly soon. One quick question, is the caching of the dom element really necessary? Seems a bit weird.

I could probably move the gh-pages branch in the master branch because GitHub now allows you to use the docs/ folder for the website.

@tarekis
Copy link
Contributor Author

tarekis commented Nov 8, 2016

It indeed looks strange, i felt like i ran ten-thousand tests until i figured out what was causing call stack overflows.

Chartists.extends hangs up on circular references when it tries to deep copy an object over another one and doesn't address circulars at all.
ES6's Object.assign() works properly and is able to handle circular references, but let's stick with Chartists.extend(); support for latter option is too bad imho, e.g. Android Browsers choke on that.
If at all, we should look into implementing circular detection into Chartists.extend()... but not right now for me.

I don't know about that one, i'd personally prefer a similar project structure as e.g. the official plugin chartist-plugin-threshold and a completely isolated project for gh-pages, just like chartist itselft. But this is just my personal preference, got the file ready, just direct me anywhere.

@SpaceK33z
Copy link
Contributor

Ohh wow, fair enough. Could you add that as a comment in the code? That way other people will also understand that.

The gh-pages branch is fine for now.

comment caching measure to prevent a stack overflow; based on #30 (comment)
@tarekis
Copy link
Contributor Author

tarekis commented Nov 8, 2016

I think that should explain the circularity problem, right?
Okay, i'm just gonna refork and PR the demo page after you've reviewed this one, i think i messed up something with branching my PR branch ... and i'm no expert in correct commit reverting 😛

@SpaceK33z SpaceK33z merged commit 9813f5b into CodeYellowBV:master Nov 13, 2016
@SpaceK33z
Copy link
Contributor

Thanks, just tested it to confirm it works 👍 . Good work! Really appreciate it. Sorry for the delay.

@SpaceK33z
Copy link
Contributor

SpaceK33z commented Nov 13, 2016

I'll release this after the docs PR :).

@tarekis
Copy link
Contributor Author

tarekis commented Nov 13, 2016

No problem, thanks! Gonna PR tommorow, code's on another machine. Will PR into gh-pages :)

@tarekis tarekis mentioned this pull request Nov 14, 2016
SpaceK33z pushed a commit that referenced this pull request Nov 14, 2016
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.

None yet

2 participants