WIP: add option for skeleton-navigation #12

PraveenGandhi · 2020-03-26T16:44:45Z

I feel that this is not a good idea (duplicate code in ./app-min and ./skeleton-navigation). Please suggest if there is a better way.

Thank you @jwx for your tutorials.

related to #5

questions.js

3cp · 2020-03-26T20:18:00Z

skeleton-navigation/src/main.ext

+import Aurelia, { RouterConfiguration/* @if shadow-dom || css-module */, StyleConfiguration }/* @endif */ } from 'aurelia';
+import { MyApp } from './my-app';
+
+Aurelia/* @if shadow-dom */.register(StyleConfiguration.shadowDOM())/* @endif *//* @if css-module */.register(StyleConfiguration.cssModulesProcessor())/* @endif */.register(RouterConfiguration).app(MyApp).start();


Yes, this file and src/resource.d.ts can be pushed down to common/ folder to avoid duplication.

I think main.ext may get nested /* if exp */
please suggest.

There is another PR going to be merged too. That will clean up main.ext, then you will only need one condition on app-with-router.
Will let you know when it's merged.

skeleton-navigation/src/my-app.html

3cp · 2020-03-26T20:20:06Z

skeleton-navigation/src/my-app.less__if_less

+  text-decoration: none;
+  color: black;
+}
+a:hover {


Less file can be cleaned up to use nested class. There are few others can be merged.

3cp · 2020-03-26T20:20:22Z

skeleton-navigation/src/my-app.scss__if_sass

+  text-decoration: none;
+  color: black;
+}
+a:hover {


Scss file can be cleaned up to use nested class. There are few others can be merged.

skeleton-navigation/src/resource.d.ts__if_typescript

skeleton-navigation/src/welcome.html

skeleton-navigation/src/welcome.ts

3cp · 2020-03-26T20:23:41Z

Thanks very much!

Please also cleanup all the files to use 2 spaces (instead of 4) on indentation.

3cp · 2020-03-26T20:25:50Z

@jwx please help to review too.

jwx · 2020-03-26T22:25:03Z

skeleton-navigation/src/about.ts

@@ -0,0 +1,3 @@
+export class About {
+


Maybe remove this file and make about an html-only custom element.

jwx · 2020-03-26T22:27:35Z

skeleton-navigation/src/my-app.css__if_css

@@ -0,0 +1,48 @@
+body {


If I'm not mistaken, this styling is more or less the styling I had in one of my live coding videos. If so, it should perhaps be both cleaned up and trimmed down for a skeleton.

you are right :), but I think minimal styling is fine to have it nice looking. Please share your thoughts.

jwx · 2020-03-26T22:27:58Z

skeleton-navigation/src/my-app.css__if_css

+.goto-active {
+  background-color: lightgray;
+}


This, however, should remain.

I think what @jwx suggested is that you can remove lots of styles and layout from the skeleton, they were only for the demonstration in his video. We can cut off, and reduce to minium, because end user just want some starting code for router, not layout and styles.

Got it, let me clean up the styling.

skeleton-navigation/src/my-app.html

jwx · 2020-03-26T22:30:10Z

skeleton-navigation/src/my-app.less__if_less

@@ -0,0 +1,48 @@
+body {


See previous comment about styling.

jwx · 2020-03-26T22:30:31Z

skeleton-navigation/src/my-app.scss__if_sass

@@ -0,0 +1,48 @@
+body {


And again about styling.

3cp · 2020-03-27T03:53:17Z

I think what @jwx suggested is that you can remove lots of styles and layout from the skeleton, they were only for the demonstration in his video. We can cut off, and reduce to minium, because end user just want some starting code for router, not layout and styles.

PraveenGandhi · 2020-03-27T03:55:23Z

@3cp ,

I think main.ext may get nested /* if exp */
please suggest.

3cp · 2020-03-27T03:56:42Z

@3cp ,

I think main.ext may get nested /* if exp */
please suggest.

Nested condition is supported. What's your intended usage (snippet)?

PraveenGandhi · 2020-03-27T04:17:00Z

I think what @jwx suggested is that you can remove lots of styles and layout from the skeleton, they were only for the demonstration in his video. We can cut off, and reduce to minium, because end user just want some starting code for router, not layout and styles.

Got it, let me clean up the styling. Shall I try bootsrap4 with font-awesome5 (similar to au1 cli generated skeleton-navigation)?

3cp · 2020-03-27T04:21:29Z

Got it, let me clean up the styling. Shall I try bootsrap4 with font-awesome5 (similar to au1 cli generated skeleton-navigation)?

Pls don't add them now :-) We want to keep this repo to minimum as au2 is a moving target.

PraveenGandhi · 2020-03-27T04:26:00Z

@3cp ,
I think main.ext may get nested /* if exp */
please suggest.

Nested condition is supported. What's your intended usage (snippet)?

As we should avoid duplication, to move main.ext to common\src ,
Is this looking fine (it generates two separate import statements from the same package)?

3cp · 2020-03-27T04:50:47Z

You can refactor the import into one with nested condition. But wait for the other PR, it will change that statement.

PraveenGandhi · 2020-03-27T05:01:22Z

@3cp ,
@jwx ,

Is this styling enough for the minimal app?

nav {
background: linear-gradient(135deg, #6d50a2 0%, #ed2b88 100%);
display: flex;
}

a {
padding: 10px;
text-decoration: none;
color: black;
}
a:hover {
background-color: darkgray;
}

.goto-active {
background-color: lightgray;
}

App looks like below:

3cp · 2020-03-27T05:18:44Z

I guess removing gradient because other part is plain, just need an obvious active style.
But that's just me. A fancy background would not hurt either!

3cp · 2020-03-27T05:22:28Z

BTW, in html, pls remove footer, and remove container div wrapper. There is no style now, no need to keep them. @jwx what's name="main" on the <au-viewport?

PraveenGandhi · 2020-03-27T06:12:55Z

@3cp ,
@jwx ,

Styling cleaned up.
Waiting for other PR to be merged to change main.ext

3cp · 2020-03-27T06:30:37Z

Not that fast :-)
There is still unit tests and cypress e2e tests need to be implemented for this feature.

PraveenGandhi · 2020-03-27T07:05:37Z

Not that fast :-)
There is still unit tests and cypress e2e tests need to be implemented for this feature.

You all rock. Take your time. I know that I am just doing small and simple things as I am a typical dev and a user of Aurelia.

Wait for aurelia/aurelia#804

…hadowDom

to-string-loader does not work with style-loader.

3cp · 2020-03-28T04:49:45Z

The other PR is merged.

3cp · 2020-03-28T05:23:03Z

Looks you got many duplicated commits.

PraveenGandhi · 2020-03-28T05:56:09Z

Looks you got many duplicated commits.

yes, how do I remove them?

3cp · 2020-03-28T06:33:34Z

Never mind, as long as it's mergable, I can squash commits into one when merging.

3cp · 2020-03-28T06:35:31Z

Or you can create a fresh branch on our master, may need to fork a repo if you are not sure how to reset your master.

PraveenGandhi · 2020-03-28T06:38:20Z

Never mind, as long as it's mergable, I can squash commits into one when merging.

Ok. Could you please review the files now?

3cp · 2020-03-28T08:17:04Z

The changes here looks messed up, they are still against the old master.
Please try to create another clean PR against current master head.

PraveenGandhi · 2020-03-28T11:47:21Z

new PR is on the way

feat: add option for skeleton-navigation

ac7ce89

3cp requested changes Mar 26, 2020

View reviewed changes

jwx reviewed Mar 26, 2020

View reviewed changes

fix: review comments addressed

fbaee1d

fix: cleanup of styling

9a64466

3cp added 7 commits March 28, 2020 15:47

refactor: use latest styles api

03f9804

Wait for aurelia/aurelia#804

fix(webpack): allow shared style import in main file for setup with s…

fcc8a5c

…hadowDom

fix(test): fix outdated test code for shadow-dom and css-modules

f111b54

chore: cleanup to avoid lint warning

a898329

fix(webpack): cleanup duplicated loader

5b2f88a

fix(webpack): fix shared shadow-dom styles

cdd50fd

to-string-loader does not work with style-loader.

fix(typescript): add missing module def for less and scss files

9947f33

PraveenGandhi added 2 commits March 28, 2020 10:41

feat: add option for skeleton-navigation

2159673

fix: review comments addressed

1502ec2

PraveenGandhi added 2 commits March 28, 2020 10:41

fix: cleanup of styling

f22e7ba

Merge branch 'master' of https://github.com/PraveenGandhi/new

6c272f8

fix: main.ext moved to common/src to deduplicate

4a26deb

PraveenGandhi closed this Mar 28, 2020

PraveenGandhi mentioned this pull request Mar 28, 2020

feat: add option for app-with-router #13

Merged

WIP: add option for skeleton-navigation #12

WIP: add option for skeleton-navigation #12

Conversation

PraveenGandhi commented Mar 26, 2020

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

3cp commented Mar 26, 2020

3cp commented Mar 26, 2020

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

3cp commented Mar 27, 2020

PraveenGandhi commented Mar 27, 2020

3cp commented Mar 27, 2020

PraveenGandhi commented Mar 27, 2020

3cp commented Mar 27, 2020

PraveenGandhi commented Mar 27, 2020

3cp commented Mar 27, 2020

PraveenGandhi commented Mar 27, 2020

3cp commented Mar 27, 2020

3cp commented Mar 27, 2020

PraveenGandhi commented Mar 27, 2020

3cp commented Mar 27, 2020

PraveenGandhi commented Mar 27, 2020

3cp commented Mar 28, 2020

3cp commented Mar 28, 2020

PraveenGandhi commented Mar 28, 2020

3cp commented Mar 28, 2020

3cp commented Mar 28, 2020

PraveenGandhi commented Mar 28, 2020

3cp commented Mar 28, 2020

PraveenGandhi commented Mar 28, 2020