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

Major Enhancements #12

Merged
merged 2 commits into from
Jan 31, 2013
Merged

Major Enhancements #12

merged 2 commits into from
Jan 31, 2013

Conversation

taitems
Copy link
Contributor

@taitems taitems commented Dec 13, 2011

List of changes summarised here: http://taitems.tumblr.com/post/14143450461/ive-just-pushed-some-pretty-hefty-changes-to-the

Commit message as follows:

  • Updated demo page doctype to HTML5
  • Moved body styles from stylesheet to page to reduce conflicts
  • Reduced conflicts with Twitter Bootstrap by namespacing code
  • Source now accepts data from a local object, not just a JSON call
  • Can now extend data properties on to bars for use with Bootstrap popovers etc
  • Code is now "use strict"
  • Fixed a couple of errors resulting from "use strict"
  • Various English translations: renamed hollydays to holidays + renamed months and days to english
  • Redesigned buttons and slider with CSS3 and an image sprite
  • Modified category colours and styles
  • Made chart 100% width so it responds to parent container width
  • Began enforcing front end dev guidelines http://taitems.github.com/Front-End-Development-Guidelines/ such as double quotations (but singles on HTML strings), triple equals comparison and using spans instead of hyperlinks for JavaScript interactivity

@mbielanczuk
Copy link
Owner

Hello taitems and ryan953
those commits are ok to merge:
507daba "Fixed all div.fn-label elements so they don't close prematurly."
b8bd512 "Merge pull request #1 from ryan953/master".

As for: 3e26458 I would like you to not change appearance of chart. Styling should be left for users so they could adjust it to their applications/sites, If you like you can provide some alternatives css/images on your site but please don't do that as regular update. I like changes you've made for english translations, which really I should do earlier :) .
Now HTML5 doctype and "use strict". I'm not convinced to those, I think we should leave this for developers that want to use this plugin.

"Source now accepts data from a local object, not just a JSON call"
For now, I will merge solution from @eneasgesing.

Thank's for collaboration, nice to know that someone else is also intrested in this project.

So, I'm waiting for changes and reply :D
Cheers
Marek

@taitems
Copy link
Contributor Author

taitems commented Jan 9, 2012

I don't think that the HTML5 doctype and "use strict" should be optional, as they enforce good coding standards from the beginning by providing developers with a good starting point. The HTML5 doctype is quite frankly the only doctype option, as it still throws IE6/7 into standards mode.

The "use strict" is great as it pinpointed areas where the code wasn't valid (so I fixed a couple of errors) and only affects the function that it's placed inside of.

I haven't seen the commit by @eneasgesing so I will have to look into that.

Thanks for the reply, I was concerned that you were no longer maintaining the project - but I'm glad you found this pull request eventually.

Tait.

@Eccenux
Copy link

Eccenux commented Jan 19, 2012

@taitems HTML5 doctype is nice but I agree this should be optional (good as a guideline thou).

And having HTML5 as a guideline... See issue #17.

@taitems
Copy link
Contributor Author

taitems commented Jan 19, 2012

I'm struggling to see the issue with doctypes. It's a demo page, it's not explicitly saying your document has to match the demo page, it's just what we're using on our particular demo.

As for "use strict", it's just not optional. Along with the HTML5 doctype, it's just how code is written these days.

I think at this point it's looking like the pull request isn't going to be successful - which is fine. I think it's more trouble than it's worth to go through and separate out commits to coding standards, graphic updates (which wouldn't be merged), doctypes and modern conventions. I'm just worried about fragmentation now. I've made so many changes throughout the file that, if @mbielanczuk makes general functional changes, or performance increases - I don't believe I'll be able to merge them automatically any more.

@ashclarke
Copy link

There are so many improvements here I could not begin to +1 every one.

@mbielanczuk mbielanczuk merged commit ae8b8a4 into mbielanczuk:master Jan 31, 2013
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.

4 participants