Skip to content

get rid of jQuery usage#39

Open
frostbolt wants to merge 2 commits into
atais:masterfrom
frostbolt:master
Open

get rid of jQuery usage#39
frostbolt wants to merge 2 commits into
atais:masterfrom
frostbolt:master

Conversation

@frostbolt

@frostbolt frostbolt commented Apr 11, 2019

Copy link
Copy Markdown

Hi.

Your plugin depends on outdated jQuery version, that is no longer maintained
At first I was going to move it to third jQuery but then I noticed that there's only one jQuery call that can be easily replaced with native code.

As you can see here, native .map realization is compatible with all modern (IE9+) browsers.

@atais

atais commented Apr 11, 2019

Copy link
Copy Markdown
Owner

dude the eonasdan-datetimepicker uses jQuery.
this is just a wrapper.

you will need jQuery anyway to run the library.

@atais atais closed this Apr 11, 2019
@frostbolt

Copy link
Copy Markdown
Author

yeah, but it uses jquery itself.
your implementation requires an outdated 2nd version, which has security vulnerabilities, according to npm audit.

It's required to get rid of it's usage or update the dependency.

@atais atais reopened this Apr 11, 2019
@atais

atais commented Apr 14, 2019

Copy link
Copy Markdown
Owner

Sorry I did not have time to merge it
however, i do not know why but travis is not showing up here.
But your changes fail on tests:
https://travis-ci.org/atais/angular-eonasdan-datetimepicker

if you fix it I can merge the change.
I think fixing the dependency is easier

@atais

atais commented Apr 24, 2019

Copy link
Copy Markdown
Owner

@frostbolt now travis works

@frostbolt

Copy link
Copy Markdown
Author

@atais I'll take a look later this month.

@frostbolt

frostbolt commented Apr 30, 2019

Copy link
Copy Markdown
Author

Hi, @atais,
Actually I found an issue in my code 🌚 , but tests still doesn't work.

When I try to run them it seems like angularjs doesn't work. I can see {{expressions}} inside the page.

But the most strange thing is that I've managed to run them manually running npm start. Looks like everything is just fine.
image

PS. I've tried to run the tests for origin/master. Unsuccessfully.

@atais

atais commented May 1, 2019

Copy link
Copy Markdown
Owner

Damn it happens from time to time because I don't update the dependencies and the tests have to be fixed.
I am currently on holidays but I will try to fix it once I am back

@frostbolt

Copy link
Copy Markdown
Author

I noticed that I've accidentally moved several dependencies from dev to common section. Fixed that.

I've also replaced ' with \" as long as 1st variant causing error on windows platform (see screenshot below).
image

@atais

atais commented May 21, 2019

Copy link
Copy Markdown
Owner

I have just looked into it, but it does not seem easy.
I do not know why it stopped working and what is the cause.

To be honest, I am not working with those technologies anymore so I really do not want to step into it.

If you want and could fix the tests I will gladly merge the changes.

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.

2 participants