Feature angular #426

shakilsiraj · 2018-07-02T00:44:30Z

Angular 5 implementation of the uikit project. Has the core and accordion features implemented so far.

Comes with the following feature:

Based on the uikit CSS framework.
Component implementation based on Angular Component Development Kit (@angular/cdk).
Animation implementation using Angular Animation library. Does not directly use @gov.au/animate but replicates the functionality using @angular/animations.
Unit testing with angular test bed
A demo project to see components in action.

More component implementation in Angular to come I hope ...

Included: 1. au-color-palette directive 2. au-outline directive

1. Changed au-color-palette to be a directive. 2. Managed to get a working test page from core project. 3. Added an option to dynamically change the color palette

* Fixed lerna bootstrap build by adding ignore to ngx project * Changed karma config to use single test runs

Changed Angular project namespace to ngx

dominikwilkowski · 2018-07-02T00:54:36Z

package.json

 		"scaffolding": "node ./scripts/helper.js scaffolding",
-		"bootstrap": "lerna bootstrap",
+		"bootstrap": "lerna bootstrap --loglevel verbose",


I think this might be an oversight? Meaning is is probably not meant for everyone to get a verbose output and was probably not meant to make it into this PR. Good for debugging, bad for performance and day-to-day use :)

yeap... my bad. I think it was from right at the beginning when I was trying to understand how lerna works

dominikwilkowski

Hi @shakilsiraj
Thank you for your PR. This looks like a lot of work. thank you for that. I don't think the DS team have the capacity to maintain yet another framework besides vanilla, react and jquery. I let them speak to that though.
Here just some feedback on my initial review:

The way we have used this mono repo is by adding each entry point for each framework to each component rather than creating new ones. That way we can reuse css etc. The way you added this in one monolithic chuck breaks this unfortunately
Introducing several new ways of building this is probably not the best strategy if you do that only in one single component. If we want to move to karma/jasmine we should do that everywhere not in one component and end up with several testing frameworks that all do the same. I for one would suggest staying with Jest and adding cypress for e2e tests to cover everything.
typescript, while awesome and amazing, is also something we should have a discussion about. I like typed languages but should probably pick one that does not diverge from the standard so we're not locked into an ecosystem (typescript has a commenting syntax that would help here)
Most of the changes in the helper are white-space changes. Best to avoid those :)
Also in general a PR with 83 files changed is probably a bit much to be merged unless clearly communicated before hand with the team. I do however totally appreciate the effort you put into this.

I let the team pick up the rest here but my personal suggestion here would be to sketch out a more aligned approach to add angular as a framework starting with the discussion how much work that adds to the design system overall. We can talk about how we might use the existing pathways to add it to each packages from there. I'm sure if that goes well we can reuse some of the work you have here.

dominikwilkowski · 2018-07-02T00:55:19Z

packages/ngx/.editorconfig

+
+[*]
+charset = utf-8
+indent_style = space


To have a conflicting editor config is probably the wrong thing to do here... Maybe we can just add prettier?

dominikwilkowski · 2018-07-02T00:56:02Z

packages/ngx/README.md

+
+## Release History
+
+* v0.4.0 - added directives


why the jump by 4 minor versions?

That was after adding few directives in the same checkin ..

dominikwilkowski · 2018-07-02T00:56:58Z

packages/ngx/karma.conf.js

@@ -0,0 +1,34 @@
+// Karma configuration file, see link for more information
+// https://karma-runner.github.io/1.0/config/configuration-file.html


Can we stay with jest rather than adding yet another test framework?

That's default test framework with Angular CLI .. nothing I can do about there ..

Angular leaves you no choice in test framework? I was not aware. That'll cause some friction later :(

dominikwilkowski · 2018-07-02T00:57:21Z

packages/ngx/package.json

+		"design guide",
+		"design system",
+		"Angular",
+    "Angular Component Development Kit",


mix of tabs and spaces?

dominikwilkowski · 2018-07-02T00:58:26Z

packages/ngx/pancake/sass/pancake.scss

@@ -0,0 +1,12 @@
+/*! PANCAKE v1.2.0 PANCAKE-SASS v1.3.0 */


pancake files and folder are artifacts of the script and need to be ignored

dominikwilkowski · 2018-07-02T01:00:59Z

packages/ngx/src/app/modules/accordion/au-accordion-group.ts

@@ -0,0 +1,14 @@
+import { Component, ContentChildren, OnInit, QueryList } from "@angular/core";
+import { CdkAccordion } from "@angular/cdk/accordion";
+// import { AuAccordionItemBase } from "./au-accordion-item";


#clean-me-up

dominikwilkowski · 2018-07-02T01:01:04Z

packages/ngx/src/app/modules/accordion/au-accordion-group.ts

+  exportAs: "auAccordionGroup"
+})
+export class AuAccordionGroup extends CdkAccordion implements OnInit {
+  // @ContentChildren(AuAccordionItemBase) contentChildren: QueryList<AuAccordionItemBase>;


#clean-me-up

dominikwilkowski · 2018-07-02T01:03:40Z

packages/ngx/tests/jquery/index.html

+
+	<h1>JQuery test: ngx</h1>
+
+	code here


since this is a angular only package it probably doesn't need jquery entry files?

dominikwilkowski · 2018-07-02T01:05:14Z

scripts/helper.js

-					if( pkg.pancake['pancake-module'].react ) {
-						react = `<a class="link" href="packages/${ module }/tests/react/">react</a>`;
+					const pkg = require(Path.normalize(`${__dirname}/../packages/${module}/package.json`));
+					if (pkg.name.indexOf('ngx') === -1) {


I'm not convinced name spacing is the best way to id packages... We have used config options in each package.json file before.

Yeah .. that one .. I had to find a way not to make an Angular library look like a React library. That was a quick and dirty solution :).

dominikwilkowski · 2018-07-02T01:13:45Z

packages/ngx/package.json

+		"@gov.au/body": "^2.0.0",
+		"@gov.au/link-list": "^2.0.0"
+	},
+	"peerDependencies": {},


dependencies have to be mirrored inside peerDependencies for pancake to work

@dominikwilkowski on your points ..

The way we have used this mono repo is by adding each entry point for each framework to each component rather than creating new ones. That way we can reuse css etc. The way you added this in one monolithic chuck breaks this unfortunately

I thought about doing the same but it would have doubled up on the packages for little benefit. Let's not even mention that there will be an angular cli build project in each of the libraries. So I have taken angular material approach - one project with multiple exportable library packages. Have a look at https://github.com/angular/material2.

This library does does not create alternative CSS for the other libraries, rather it requires those CSS files to be included manually (https://github.com/shakilsiraj/uikit/blob/feature-angular/packages/ngx/src/sass/_module.scss) into Angular projects that use this library. So, I don't think it's an issue.

Introducing several new ways of building this is probably not the best strategy if you do that only in one single component. If we want to move to karma/jasmine we should do that everywhere not in one component and end up with several testing frameworks that all do the same. I for one would suggest staying with Jest and adding cypress for e2e tests to cover everything.

If you are going to support Angular cli based projects then there is no other way but to go with Karma/Jasmine. A single project with this exception(not using Jest and use the appropriate libraries that are required) will keep it under control. Jest is a good framework no doubt but it's an added library that you will need to support on top of what Angular is providing. In Angular world, you can try to not use Karma/Jasmin, but you will be doing extra work and will need to maintain that work for yourself.

typescript, while awesome and amazing, is also something we should have a discussion about. I like typed languages but should probably pick one that does not diverge from the standard so we're not locked into an ecosystem (typescript has a commenting syntax that would help here)

Angular does not support vanilla JS (directives, enums, proper OOP, etc. are missing there). So, there is not alternative.

Also in general a PR with 83 files changed is probably a bit much to be merged unless clearly communicated before hand with the team. I do however totally appreciate the effort you put into this.

Thanks for that and sorry for not communicating before hand. Most of those files in this PR would be standard from angular CLI when you start a project. The bit that you should be focusing on is https://github.com/shakilsiraj/uikit/tree/feature-angular/packages/ngx/src/app/modules. Next PRs from anyone should be small.

Happy to discuss.

pattyde · 2018-07-02T01:35:31Z

Thanks for the PR @shakilsiraj, we really appreciate all the work that's gone into this!

And thanks for the review @dominikwilkowski :)

Our lead developer is back from holiday in a couple days and we'll have a more detailed look at the code then. This will be a great opportunity to discuss how we might support another JS framework in the design system. I know there are a few agencies that are currently using Angular so this is a good conversation to have.

shakilsiraj · 2018-07-27T05:25:17Z

Hey @alex-page, thanks for your comment.

I had a close look at the other tools (furnace, pancake) that you have been talking about and now I understand how they hang together. Some cool stuff you guys have there :).

Unfortunately, with this setup, I can't see how you can integrate Angular without bloating the existing packages. Angular is a beast by itself and requires an entire CLI project to be created just for a small library. If I go with the Angular pancake module, I will need to create a separate CLI project like the ngx one in each of the modules. That will slow down installation and build speed significantly especially for those who are not working with Angular.

A better alternative perhaps is to start fresh with a separate Angular project and pull in the required UIKIT projects via npm dependencies. That would be cleaner and easy to maintain. This can be hosted on a separate repo on its own. I'm sure there are enough people to support the project.

Happy to discuss.

alex-page · 2018-07-27T06:42:16Z

Thanks for the positive vibes and understanding @shakilsiraj definitely keen to talk more about this. Feel free to send an email to [email protected] and we can organise a meeting in person or on the phone!

I'm about to head off for the day, so i'll try give you a better reply about the approach for angular components on Monday 👍

alex-page · 2018-07-30T05:57:33Z

@shakilsiraj Ok I have some time to reply now. Again this pull request is awesome and really appreciate all of your hard work.

If I go with the Angular pancake module, I will need to create a separate CLI project like the ngx one in each of the modules. That will slow down installation and build speed significantly especially for those who are not working with Angular.

So this shouldn't be as big of an issue as you think. We have ways to enable and disable the build pipelines in pancake. This means that a user who wants angular would be able to get angular components in a folder. Users who do not want angular will not be affected.

The components themselves and the libraries that make them work are actually separated. When we deliver a react component we deliver pure component code but no library to make it work. When a user downloads a component and wants to use the react pipeline in pancake, they would literally only get the component file ( accordion.js ). We rely on the user to install react, npm, node and import the component in to their project. The same would also happen for angular we would expect the user to install angular or set up a cli that can then ingest the angular components.

I'll see if I can do a similar thing with a simple angular component and share it back to you and see what you think.

One thing we do is create a test page for each component. This would require angular to be added as a developer dependency. This would not be included as a dependency when installing the components.

A better alternative perhaps is to start fresh with a separate Angular project and pull in the required UIKIT projects via npm dependencies. That would be cleaner and easy to maintain. This can be hosted on a separate repo on its own. I'm sure there are enough people to support the project.

I think this is a valid solution, however I would love angular to be in this library and supported by the community. Splitting the projects for now may mean that you can work faster and are not blocked by us. If you do choose to go down this path I would love to one day soon sit down and discuss your approach and learnings and see how/if we can merge the two together.

shakilsiraj · 2018-08-03T04:23:28Z

Hey @alex-page, I did send you an email with my contact number to [email protected]. Just wondering if you have received it or not? Love to have a chat. Thanks.

alex-page · 2018-08-06T04:16:30Z

@shakilsiraj received cheers 👍

alex-page · 2018-08-07T05:04:30Z

Hey @shakilsiraj thanks for the chat today. For everyone else's benefit here are somethings that came out of it.

We need the components to work with whatever bundler the user is using. This means it should work with rollup, angular cli or webpack. This also means that when we build an angular component it needs to be a pure component ( one javascript file that gets imported ) that should work in any build pipeline.

For our test pages we will need to use a build pipeline to actually render the angular component to the page, something like angular-cli. This would only be included as a developer dependency for this task and the user would be able to choose other implementations to suit themselves.

Things we and @shakilsiraj will try and do this sprint:

Set up the helper.js file to do an angular pipeline to build from src -> lib
Build a simple component direction-links/src/js/angular.js in angular
Render this to a test page
Publish a next version of the component and test how it integrates with an external angular pipeline
Start writing technical articles explaining how to get started with the design system using HTML, react and angular

shakilsiraj · 2018-08-08T01:24:04Z

Hi guys,

It was nice chatting with you yesterday.

I think I have a good idea now in terms of bundling Angular components with rollup and test with jest. But I would like to get clarification re the build process that will generate the angular package to be consumed by apps. When I looked into @gov.au/accordian, I can see that the main entry in package.json points to 'lib/js/react.es5.js' even though the actual lib directory has the JQuery version. I guess for JQuery, you can just refer this file from html script tag, but it will be better to point to the angular lib file so the compiler can do a proper AOT compilation in the consuming app. How would you manage other library files (for example, lib/js/ngx.es5.js) to be exposed as the main entry point? Is there going to be another package created like @gov.au/accordian-ngx by the furnace tool?

The other option is to use a proper Angular library generator like this one - https://www.npmjs.com/package/ng-packagr. It will create an entire dist directory with the required bundles (es5, es2015, esm5, package.json, etc.) that can be used for publishing to npm directly. Can furnace create a separate angular package from this dist directory?

Happy to discuss. Thanks.

alex-page · 2018-08-08T02:19:11Z

Hey @shakilsiraj you make a really good point!

I think for now we can just import it with the path to the file, as we can only have one default file for now 😢 . Happy to look into other solutions to make this easier in the future. 👍

shakilsiraj · 2018-08-14T09:15:50Z

I think(????) I have managed to get a good crack at it. Check out the the commit 0eeb2b4 in regards to #471 and #472.

Gist:

There is an angular implementation under src/ts directory.
There is a barebone angular demo app for the component under test/angular director.
Jest with angular preset has been used to run unit tests - see setup-jest.ts and jest definition in package.json as well as test:unit-test target
build:angular would create the distribution lib at lib/js/angular.es2015.js.
test:angular would compile the demo site under test/angular directory.

I'm guessing you guys would be able to work out #470 and #473 from the fork? I might have missed something here, so would appreciate you guys having a go at it. Please let me know if you have any questions.

alex-page · 2018-08-30T02:48:15Z

Thanks @shakilsiraj I'm going to look at your work over the next few days and see what we can do to get this integrated correctly!

shakilsiraj · 2018-08-30T05:09:12Z

👍

alex-page · 2018-08-31T03:36:51Z

Hi @shakilsiraj I think I have an idea about how this might be able to work from a simple approach. Let me know what you think.

The uikit could provide a template html and css file for ingestion by angular. This means that the html file could look like:

<div class="au-callout {{ classnames }}">{{ text }}</div>

Together they could be ingested like:

import { Component } from '@angular/core';

@Component({
  selector: 'au-callout',
  templateUrl: './au-callout.html',
  styleUrls: ['./au-callout.css']
})

export class AUcallout {
  text: 'a string or a variable',
  classnames: ' a string or variable',
}

Then our test pages could be a simple angular-cli repository ( however with a lot of the fuzz removed ).

The main thing I am not sure about is dynamic components, we can provide the spot for the class to go
and click handlers however we wouldn't be writing the functionality in this case. Which may be beneficial of developers who want to control the state in other ways.

Let me know if you have any suggestions or ideas, i'm trying to keep the changes as minimal as possible so we can get this work in easily.

shakilsiraj · 2018-09-02T00:40:12Z

Hey @alex-page,

I think I have done something very similar. Have a look at the following:

The angular module:
https://github.com/shakilsiraj/uikit/tree/feature-angular/packages/direction-links/src/ts

The test application ( a barebone version of an ng cli project:
https://github.com/shakilsiraj/uikit/tree/feature-angular/packages/direction-links/tests/angular/src

And the changes to package.json(https://github.com/shakilsiraj/uikit/blob/feature-angular/packages/direction-links/package.json) along with the new build targets: test:angular and build:angular.

What do you think?

alex-page · 2018-09-02T23:14:36Z

@shakilsiraj Yeah exactly something like this, I might try making a new branch with your code and see if I can make it a little bit smaller.

Thanks again for all your work and feedback!

alex-page · 2019-07-31T19:06:38Z

@sukhrajghuman are you able to remove me as reviewer 🙏

sukhrajghuman · 2019-07-31T23:39:53Z

@alex-page doesn't look like I can... :(

@shakilsiraj might be able to

dominikwilkowski · 2019-07-31T23:44:42Z

@alex-page you're not a reviewer (or what GitHub calls it assignee) anymore and it only shows your last review on the top. You can just mute the issues for yourself. :)

shakilsiraj added 25 commits April 25, 2018 02:09

core-ng : Initial project setup

6f300b6

core-ng : Created angular app and library.

309358b

Included: 1. au-color-palette directive 2. au-outline directive

code cleanup

1c61f32

fixed build script

7d80b8c

Added au-focus directive

c882cce

Added au-focus directive

33fcc36

Fixed formatting

12952af

More fixes:

04c8941

1. Changed au-color-palette to be a directive. 2. Managed to get a working test page from core project. 3. Added an option to dynamically change the color palette

added au-fontgrid and cleanup

0a37a30

added au-fontgrid and cleanup

50e2a02

added au-fontgrid and cleanup

0b5a0fe

added au-body and refactored the project name and folder

5154ff0

added au-body and refactored the project name and folder

33fec78

Fixed build

ca5b02a

* Fixed lerna bootstrap build by adding ignore to ngx project * Changed karma config to use single test runs

Changed namespace

2350ae2

Changed Angular project namespace to ngx

added accordion

86addcf

added enter key down listener for accordion header

53690aa

more changes...

61c1b18

More work ..

c4bddfd

accordion fixes

54d8aea

accordion fixes

977b38d

More work ..

435ecd0

Fixing accordion group multi expansion logic

98a5dbc

Added angular animation for accordion

74bc843

Bumping version number

6ceb9ae

dominikwilkowski reviewed Jul 2, 2018

View reviewed changes

dominikwilkowski suggested changes Jul 2, 2018

View reviewed changes

pattyde assigned alex-page Jul 2, 2018

pattyde requested a review from alex-page July 2, 2018 01:35

alex-page assigned shakilsiraj Jul 11, 2018

shakilsiraj and others added 2 commits August 14, 2018 18:47

Angular implementation of direction-link

0eeb2b4

Delete pancake.scss

1065d96

shakilsiraj unassigned alex-page Feb 21, 2019

azerella force-pushed the develop branch 5 times, most recently from c343bcc to 1b6a5f2 Compare March 1, 2019 00:52

sukhrajghuman requested review from KiriHoy and removed request for KiriHoy July 31, 2019 23:06

		@@ -0,0 +1,34 @@
		// Karma configuration file, see link for more information
		// https://karma-runner.github.io/1.0/config/configuration-file.html

Feature angular #426

Are you sure you want to change the base?

Feature angular #426

Conversation

shakilsiraj commented Jul 2, 2018

dominikwilkowski Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikwilkowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattyde commented Jul 2, 2018

shakilsiraj commented Jul 27, 2018

alex-page commented Jul 27, 2018

alex-page commented Jul 30, 2018 • edited Loading

shakilsiraj commented Aug 3, 2018

alex-page commented Aug 6, 2018

alex-page commented Aug 7, 2018

shakilsiraj commented Aug 8, 2018

alex-page commented Aug 8, 2018 • edited Loading

shakilsiraj commented Aug 14, 2018

alex-page commented Aug 30, 2018

shakilsiraj commented Aug 30, 2018

alex-page commented Aug 31, 2018 • edited Loading

shakilsiraj commented Sep 2, 2018

alex-page commented Sep 2, 2018

alex-page commented Jul 31, 2019

sukhrajghuman commented Jul 31, 2019 • edited Loading

dominikwilkowski commented Jul 31, 2019

dominikwilkowski Jul 2, 2018 •

edited

Loading

alex-page commented Jul 30, 2018 •

edited

Loading

alex-page commented Aug 8, 2018 •

edited

Loading

alex-page commented Aug 31, 2018 •

edited

Loading

sukhrajghuman commented Jul 31, 2019 •

edited

Loading