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

Trident web browser control compat #133

Merged

Conversation

mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Jan 2, 2020

@reddyashish @QilongTang
Here are the changes I had to make to librarieJS to support this PR:

DynamoDS/Dynamo#10241

I prefer to keep this a separate branch for now, wanted to generate the diff for review.

…o look it up later to reload the correct image.

add core-js for ie polyfills (startsWith) - we can try just using a polyfill for that method only later.
add core-js to entrypoint so its imported
@@ -0,0 +1 @@
registry=https://art-bobcat.autodesk.com/artifactory/api/npm/autodesk-npm-virtual/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we make this change before already?

Copy link
Member Author

@mjkkirschner mjkkirschner Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but only on the develop branch.... which I think we should get rid of - all development should be on master to avoid things getting out of sync.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it already is :( out of sync.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjkkirschner Thx for the reminder!

@@ -4,6 +4,8 @@ import * as ReactDOM from "react-dom";
import { LibraryContainer } from "./components/LibraryContainer";
import { JsonDownloader } from "./LibraryUtilities";
import { Reactor, Event } from "./EventHandler";
import "core-js";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mjkkirschner I did not understand the purpose from your commit message, can you elaborate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core-js includes a bunch of polyfills - so that browsers (like ie) which are missing some functions can use them - in this case I needed string.prototype.startsWith but we could probably just add the polyfill for that single method.... this just works though.

@@ -179,6 +179,7 @@ export class LibraryItem extends React.Component<LibraryItemProps, LibraryItemSt
}

onImageLoadFail(event: any) {
event.target.orgSrc = event.target.src;
Copy link
Member Author

@mjkkirschner mjkkirschner Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this lets us lookup what the original Image.Src url was for an icon, even if we have fallenback to the default dynamo icon.

Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@zeusongit zeusongit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mjkkirschner
Copy link
Member Author

mjkkirschner commented Jan 6, 2020

@QilongTang @zeusongit @reddyashish do you guys have a preference between merging this into master or to keeping it on a separate branch?

@aparajit-pratap
Copy link
Contributor

I am for having to maintain one version of librarie.js as far as possible so yes, I would go for master.

@QilongTang
Copy link
Contributor

master is fine with me

@mjkkirschner
Copy link
Member Author

tests are all passing on this branch. I am going to file a task to delete the develop branch and merge it to master.

@mjkkirschner mjkkirschner merged commit 9b81fe6 into DynamoDS:master Jan 9, 2020
QilongTang added a commit that referenced this pull request Feb 24, 2020
* Create .sonarcloud.properties

* Trident web browser control compat (#133)

* cache the old url when the image fails to load - we need to be able to look it up later to reload the correct image.
add core-js for ie polyfills (startsWith) - we can try just using a polyfill for that method only later.
add core-js to entrypoint so its imported

* remove discarded changes for extra event

* Changes to use cbp/stable branch for the librariejs

* Changes to use the CI Lib  using the credential feature

Co-authored-by: Aaron (Qilong) <[email protected]>
Co-authored-by: Michael Kirschner <[email protected]>
QilongTang added a commit that referenced this pull request Mar 12, 2020
* Fix tests (#124)

* Fix test

* Update README.md

* Forcing version by package.json

* Add pipeline to run test in docker image. (#125)

* Add pipeline to run test in docker image.

* Create .npmrc

* Update README.md

* Update README.md

* Update README.md

* Update package.json

* Update package.json

* Update Jenkinsfile

* Slack notification and docker suport (#126)

* Delete duplicate folder (#127)

* Add missing files

* Delete indent-arrow-down-wo-lines.svg

* Delete indent-arrow-right-wo-lines.svg

* Delete plus-symbol.svg

* Update package.json (#128)

* Update pipeline.yml for new Artifactory url (#129)

* Update package.json

* Update pipeline.yml

* Update pipeline.yml

* Update pipeline.yml

* Update pipeline.yml

* Update pipeline.yml

* Update pipeline.yml

* New docker image

* Add of csproj file for nuget packing (#130)

* add of csproj file for nuget packing

* New sources for dotnet pack

* Update pipeline.yml

* Update pipeline.yml

* Update pipeline.yml

* Update librariejs.csproj

* Update librariejs.csproj

* Update librariejs.csproj

* Change name of project

* new NUGET_API_ID (#131)

* CI to switch to use stable branch (#136)

* Create .sonarcloud.properties

* Trident web browser control compat (#133)

* cache the old url when the image fails to load - we need to be able to look it up later to reload the correct image.
add core-js for ie polyfills (startsWith) - we can try just using a polyfill for that method only later.
add core-js to entrypoint so its imported

* remove discarded changes for extra event

* Changes to use cbp/stable branch for the librariejs

* Changes to use the CI Lib  using the credential feature

Co-authored-by: Aaron (Qilong) <[email protected]>
Co-authored-by: Michael Kirschner <[email protected]>

* Change of credential ID to use custom docker image (#137)

* Develop (#139)

* Change of credential ID to use custom docker image

* Remove additional line of nprmc

Co-authored-by: jesusHCG <[email protected]>
Co-authored-by: manuelsaldivar525 <[email protected]>
Co-authored-by: Aaron (Qilong) <[email protected]>
Co-authored-by: Michael Kirschner <[email protected]>
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