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

multiple various fixes, refactoring, add all echarts events #154

Merged
merged 1 commit into from
Dec 16, 2018

Conversation

smnbbrv
Copy link
Contributor

@smnbbrv smnbbrv commented Dec 10, 2018

Hi @xieziyu

recently I needed the legend events and I recognised they were not yet supported. So, I started adding them, and, well, found lots of improvements I can perform :)

So, I took some time and added lots of minor / major fixes / refactorings / improvements. Here is the list:

  • add all events
  • switch to dynamically bound lazy events, see https://stackoverflow.com/a/53704102/1990451
  • remove all events file. This is simply unnecessary, creates more complexity while maintaining multiple events
  • deprecate the detectEventChanges (it is simply ignored now). When you decide to move to the new major version it could be killed
  • consolidate variable naming. E.g. some privates were called with _, some not. => remove _ in the beginning of the private attributes (they are already private), etc.
  • switch window resize event from host binding (that has no sense at all because the host has nothing to do with window) to rx fromEvent and get rid of the resize observable (because it is simply unnecessary)
  • create a separate dispose method that is called to reinitialise the chart instance. The resize subscription is not resubscribed anymore and matches the component lifetime
  • update the events example
  • add ignoring the import lines for tslint max-length rule

Please share what do you think. I know, it is probably too many changes and perhaps could be done slower within multiple commits / PRs...

@xieziyu
Copy link
Owner

xieziyu commented Dec 11, 2018

@smnbbrv
Excellent! Your solution is amazing. I love that createLazyEvent style. Thank you.

I need some time to review the changes, will merge it in this week.

@smnbbrv
Copy link
Contributor Author

smnbbrv commented Dec 11, 2018

thanks @xieziyu :) Take your time, this can definitely wait, better to make it cold :)

@xieziyu xieziyu merged commit 0e5f795 into xieziyu:master Dec 16, 2018
@gabrielberlanda
Copy link

Hello Guys,

First of all thanks for this plugin,

I'm currently using it in my ionic app, and the ng version homologated in my ionic version is 5.2.11, so I have to use the old version of your plugin, I'm wondering if have a way to merge this fixes in the oldest version (2.3.1) too?

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

3 participants