Skip to content

Conversation

@Dewb
Copy link

@Dewb Dewb commented Aug 21, 2020

  • Update for node 12, set engine property in package.json
  • Fix npm audit issues
  • Run npx uneject to restore create-react-app webpack config, reduce direct dependencies
  • Remove other unused dependencies
  • Fix security warnings in Chrome console
  • Fix mixed-standard module exports


## Deploy to Autodesk (Staging)

- Confirm that `homepage` in `package.json` is set to `http://staging-dictionary.dynamobim.com/`. The developer may change this address depending on the staging environment.
Copy link
Member

Choose a reason for hiding this comment

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

were these docs moved somewhere internal? I think @zeusongit just documented these here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually it's not a good idea to make staging environments public, so the change makes sense.

var ml = [];
ad.forEach(function (d, i) {
var c = d.Categories;
var parent = "Home";
Copy link
Member

Choose a reason for hiding this comment

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

could you comment on this change?

Copy link
Author

Choose a reason for hiding this comment

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

This eliminates an unused variable warning in the development server console.

Copy link
Collaborator

@mmisol mmisol left a comment

Choose a reason for hiding this comment

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

Only one minor text issue related to changes here.
Other that that:

  • The "Create Pull Request" functionality hangs for me, but I'm not sure if this ever worked when running local.

  • Another thing I noticed is editing the description of a node does not show up as a change in the "Create Pull Request" dialog. Should it?

  • The unit test is still failing, but it was failing already, so no change there I guess.


## Deploy to Autodesk (Staging)

- Confirm that `homepage` in `package.json` is set to `http://staging-dictionary.dynamobim.com/`. The developer may change this address depending on the staging environment.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually it's not a good idea to make staging environments public, so the change makes sense.

<a href="http://dynamoprimer.com/" target="_blank" rel="noopener noreferrer" style={{'color':'orangered'}}>Dynamo Primer</a>,
this dictionary is open-source - check it out on our
<a href="https://github.com/DynamoDS/DynamoDictionary" target="_blank" rel="noopener noreferrer" style={{'color':'orangered'}} >Github page</a>
and contribute!</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this part, the text shows next to the link, without spaces separating them:
Screen Shot 2020-08-21 at 8 58 06 AM

Fixes a spacing issue between the text and the links in the home page.

Also fixes the only unit test, by creating the App component in the
same way it's created by the actual application.
@mmisol mmisol merged commit 9bfc7ac into master Aug 24, 2020
@QilongTang QilongTang deleted the dewberm/modernize branch September 24, 2020 15:29
@QilongTang
Copy link

QilongTang commented Oct 14, 2020

@Dewb Back to this PR late, would you explain Run npx uneject to restore create-react-app webpack config, reduce direct dependencies? Are we still using webpack here? I am looking at a legacy PR #22 which bring in another webpack package html-loader as dependency and trying to figure out the conflict after this PR and merge that in. Appreciate that.

npx uneject failed for me locally

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.

6 participants