patternfly / angular-patternfly

This repo contains instructions and the code for a set of Angular 1 components for the PatternFly project.
http://www.patternfly.org/angular-patternfly/
MIT License
122 stars 90 forks source link

Fix external fetch #712

Closed jonathanchristison closed 6 years ago

jonathanchristison commented 6 years ago

Description

This PR replaces the components/jqueryui shim repository with jquery official npm package, this fixes issues where airgap build infrastructure might be used as the inclusion of components/jqueryui will make an external request to be made to github -
components-jqueryui@1.12.1 (git://github.com/components/jqueryui.git#44ecf3794cc56b65954cc19737234a3119d036cc)

Briefly looking at the code jquery-ui should be a drop in replacement but I'm lacking any examples to test this, if there are any test projects where i might be able to identify any visual or breaking changes please let me know and I'll try and verify the changes

PR Checklist

dtaylor113 commented 6 years ago

Hi @jonathanchristison, the pfCanvasEditor uses jquiery-ui:

https://www.patternfly.org/angular-patternfly/#/api/patternfly.canvas.component:pfCanvasEditor#example_demo

Click on the [Add Item] button to open up the Canvas Toolbox. jqyoui-draggable & jqyoui-droppable are used when one drags an Item from the toolbox and drops it onto the Canavs. Please test this component. -thanks!

jonathanchristison commented 6 years ago

@dtaylor113 Thanks for that, turns out it totally didn't work 😛 or rather was producing a load of errors in the console, the drag and drop functionality still seemed to work but I'm not sure how.

I discovered the jquery-ui package itself doesn't have any "prebuilt" elements and if your not using their AMD approach its pretty difficult to import the relevant .js files in the ngdocs task. I started to look into either calling the grunt build task in the jquery-ui package or crowbarring requirejs into the toplevel grunt file but both of those approaches seemed really hacky.

The jquery-ui-dist package is the drop-in replacement i had originally presumed jquery-ui to be and does avoid the original external git request the shim repository was introducing.

As i mentioned i was able to eliminate any errors in the firefox console, however i was also unable to break the drag & drop functionality with my original changes so i'm not sure how to best test this, visually its identical.

dtaylor113 commented 6 years ago

I just downloaded and ran your branch, the pfCanvasEditor seems to be working fine with the new jquery lib. No errors shown in my console (Chrome).

jonathanchristison commented 6 years ago

So are you saying it works in your app which consumes angular-patternfly but doesn't work in the ngDocs example?

It was working in ngDocs but i'm guessing could of been a browser cache issue, it didn't seem to break the functionality but the console was full of errors, I fixed that with my last commit which uses jquery-ui-dist opposed to jquery-ui.

To give a bit more of a background this is to fix build issues internally with hawtio @akieling will probably be able to tell you more about how angular-patternfly is used in hawtio upstream and its fuse fork but the point is to reduce external fetches which are almost impossible to override within npm/yarn - npm sill tryHTTPS attempting to clone git+https://github.com/components/jquery-ui.git

The aim of this PR is to remove this external fetch with minimal impact on users, you're obviously in a better position to judge if this change is too disruptive

dtaylor113 commented 6 years ago

Cloned this PR branch, running locally with jquery-ui-dist, pfCanvasEditor's drag-n-drop seems to working as expected, no errors, so I'd be inclined to accept this PR if he helps resolve your npm/yarn issues.

amarie401 commented 6 years ago

Ran the branch locally and works fine for me as well.