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

feat(): upgrade to [email protected] and [email protected] #967

Merged
merged 16 commits into from
Nov 13, 2017

Conversation

emoralesb05
Copy link
Contributor

Description

Following the breaking changes from https://github.com/angular/material2/releases/tag/5.0.0-rc0, this will give covalent the 5.0.0-rc0 support plus [email protected] support

What's included

  • chore(): upgrade material/cdk dependencies to 5.0.0-rc0
  • chore(): upgrade angular dependencies to 5.0.0
  • chore(): upgrade angular/cli dependencies to 1.5.0
  • fix(): breaking change replace MatDrawerToggleResult with void return
  • feat(): breaking change from angular 5 we have to register specific locale
  • fix(): remove deprecated methods and classes

Test Steps

  • yarn install
  • ng serve
  • See all still working

General Tests for Every PR

  • ng serve --aot still works.
  • npm run lint passes.
  • npm test passes and code coverage is not lower.
  • npm run build still works.

@Josh-Schoen
Copy link
Collaborator

Josh-Schoen commented Nov 10, 2017

Getting an error on npm run build Anyone else seeing this?
TypeError: require.extensions.hasOwnProperty is not a function
at requireDir (/Users//Documents/teradata/repos/covalent/node_modules/require-dir/index.js:97:37)
at Object. (/Users//Documents/teradata/repos/covalent/gulpfile.js:3:23)
at Module._compile (module.js:635:30)
at Object.Module._extensions..js (module.js:646:10)
at Module.load (module.js:554:32)
at tryModuleLoad (module.js:497:12)
at Function.Module._load (module.js:489:3)
at Module.require (module.js:579:17)
at require (internal/module.js:11:18)
at Liftoff.handleArguments (/Users//Documents/teradata/repos/covalent/node_modules/gulp/bin/gulp.js:116:3)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] build: `bash scripts/build-release
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR! /Users//.npm/_logs/2017-11-10T04_18_09_306Z-debug.log

@Josh-Schoen
Copy link
Collaborator

updating the dev dependency "require-dir": "0.3.0" to "require-dir": "0.3.2" solved it for me.

@Josh-Schoen
Copy link
Collaborator

Josh-Schoen commented Nov 10, 2017

Also found an issue with dev dependency "node-sass": "3.8.0" think it's related to this after a npm install.

 Cannot
download "https://github.com/sass/node-sass/releases/download/v3.8.0/darwin-x64-57_binding.node":

HTTP error 404 Not Found

There were a bunch more errors but changed to "node-sass": "4.6.0" and everything worked. However, I did notice that the Nav Drawers in the layout section got jankey. See the gif below, not sure if it's related... I'm running running node v8.9.1 and npm 5.5.1.
sidenav-issue

@emoralesb05
Copy link
Contributor Author

Whoops, good catch @Josh-Schoen . I think that was introduced with the layout.scss changes.

@emoralesb05
Copy link
Contributor Author

Ah nvm, i think its the mat-toolbar breaking change. They removed the implicit mat-toolbar-row

@emoralesb05
Copy link
Contributor Author

fixed~

@JoshSchoen
Copy link
Contributor

@emoralesb05 do you see any issues with the dev dependencies listed above?

@ianqueue
Copy link

Noticing an issue with Grouped Expansion Panels.

Reproducible by selecting the code tab, then navigating back to Demo tab.

screen shot 2017-11-10 at 2 49 25 pm

screen shot 2017-11-10 at 2 49 49 pm

@emoralesb05
Copy link
Contributor Author

@ianqueue thats actually an issue thats happening since [email protected].. it happens with any animation inside another animation. in this case td-expansion-panel animation inside the mat-tabs animation

@Josh-Schoen i didnt run into that issue.. not sure why you are running into it

@joshwiens
Copy link
Contributor

joshwiens commented Nov 12, 2017

Just adding a note here in the off chance it comes of. For anyone using Jest to test, a workaround is required in your Global Mocks related to animations. Specifically, the tdRotate animation used in td-expansion-panel

Object.defineProperty(document.body.style, 'transform', {
  value: () => {
    return {
      enumerable: true,
      configurable: true
    };
  },
});

Credit to @marcojahn for the workaround.

@ianqueue
Copy link

@emoralesb05 - the difference is somewhat nuanced for sure... I'm comparing what is in prod now and they behave differently fwiw regarding the expansion panels. You can see the difference if you navigate between Demo and Code tabs here... https://teradata.github.io/covalent/#/components/expansion-panel

Basically they lose state on this branch... but not in master ( may be furtherly broken as you stated ). Good to know about the animations.

@richavyas
Copy link
Collaborator

richavyas commented Nov 13, 2017

Have we changed favicon as part of this or any previous PRs as I don't see covalent
covalent favicon.

Update : it was probably just the browser cache because I don't see after multiple screen clicks.

Copy link
Collaborator

@richavyas richavyas left a comment

Choose a reason for hiding this comment

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

Text distortion happening on changing screen resolution for Data Table.

covalent-text-distortion

@richavyas
Copy link
Collaborator

Same text distortion issue on dynamic-forms

@richavyas
Copy link
Collaborator

Form input extending outside the panel

form-input

@emoralesb05
Copy link
Contributor Author

Cool, was fixing the dynamic forms one so we are good to go on that one, i dont see the data-table one though

@emoralesb05
Copy link
Contributor Author

Ah, you mean the data-table API docs? yeah, we need to change that into an mat-list

Copy link
Collaborator

@richavyas richavyas left a comment

Choose a reason for hiding this comment

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

LGTM

@emoralesb05 emoralesb05 merged commit ba18ef5 into develop Nov 13, 2017
@emoralesb05 emoralesb05 deleted the feature/material-rc branch November 13, 2017 23:12
@dapriett
Copy link

Nice - any idea when this will get published to npm?

@joshwiens
Copy link
Contributor

You can use the nightly builds for the time being

@lincze
Copy link

lincze commented Nov 14, 2017

will the node < 7 constraint be lifted (i've tried a build and found at the 1st sight no problem)?

I' made these changes (at commit ba18ef5 from 'develop')

diff --git a/package.json b/package.json
index 69684d2f..9abb7104 100644
--- a/package.json
+++ b/package.json
@@ -35,7 +35,7 @@
     "combat-training": "bash scripts/combat-training"
   },
   "engines": {
-    "node": ">=6.11.1 < 7",
+    "node": ">=6.11.1 < 9",
     "npm": ">3"
   },
   "repository": {
@@ -122,10 +122,10 @@
     "karma-jasmine-html-reporter": "^0.2.2",
     "karma-phantomjs-launcher": "1.0.1",
     "merge2": "1.0.2",
-    "node-sass": "3.8.0",
+    "node-sass": "4.6.1",
     "phantomjs-prebuilt": "2.1.14",
     "protractor": "~5.1.0",
-    "require-dir": "0.3.0",
+    "require-dir": "0.3.2",
     "rollup": "^0.41.6",
     "rollup-plugin-commonjs": "8.0.2",
     "rollup-plugin-node-resolve": "^3.0.0",
[incze@hp6000 covalent]$

(I don't know that node-sass update was necessary or not, but anyway, that was installed. the 'require-dir' bump is needed to 'npm run build')

with this config I ran 'npm run reinstall:latest' (I don't use yarn yet), and after that, both 'ng serve' and 'npm run build' worked as expected. my setup (+ npm 5.5.1):

[incze@hp6000 covalent]$ ng version

    _                      _                 ____ _     ___
   / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
  / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
 / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
/_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
               |___/
    
Angular CLI: 1.5.0
Node: 8.9.1
OS: linux x64
Angular: 5.0.1
... animations, common, compiler, compiler-cli, core, forms
... http, platform-browser, platform-browser-dynamic
... platform-server, router

@angular/cdk: 5.0.0-rc0
@angular/cli: 1.5.0
@angular/material: 5.0.0-rc0
@angular-devkit/build-optimizer: 0.0.32
@angular-devkit/core: 0.0.20
@angular-devkit/schematics: 0.0.35
@ngtools/json-schema: 1.1.0
@ngtools/webpack: 1.8.0
@schematics/angular: 0.1.2
typescript: 2.4.2
webpack: 3.8.1

@emoralesb05
Copy link
Contributor Author

emoralesb05 commented Nov 14, 2017

@lincze thats a restriction we do on our repo since we dont want people developing features use anything over node 7.. you can use whatever you want in your projects that use covalent since its package.json based.

kriswinbush pushed a commit to kriswinbush/covalent that referenced this pull request Feb 20, 2020
* chore(): upgrade to [email protected] and material 5.0.0-rc

* fix(): add test.ts to spec.json

* fix(): breaking change replace MatDrawerToggleResult with void return

* chore(): add TrackByFunction type to virtual scroll

* fix(): remove deprecated methods and classes

* feat(): breaking change from angular 5 we have to register specific locales now

* fix(): test registered and unregistered locales

* fix(): fix navigation-drawer layout alignment because of material breaking change

* chore(): add aliases for `aot` and `reinstall:latest`

* fix(data-table): fixed clickable demo

* fix(menu): add height:auto to lists inside td-menu

* fix(dynamic-forms): fix input width for demo

* chore(): add node-sass rebuild for yarn reinstall
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants