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(docs-infra): add getting started widgets #26059
feat(docs-infra): add getting started widgets #26059
Conversation
You can preview 2b8eaa9 at https://pr26059-2b8eaa9.ngbuilds.io/. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests seem a little off. I didn't look through all of them carefully. The rest looks reasonable, but it would be better if we could see them in action 😉
Some observations:
- Too much observables. Often seem unnecessary.
- It would be nice if we could get rid of
.template
and derive it from the.result
HTML. (That would make them more future proof too.) - If we expect to have more of these components in the future, it might make sense to make them more generic/configuration based. But they are fine for now imo.
aio/src/app/custom-elements/getting-started/container/container.module.ts
Outdated
Show resolved
Hide resolved
aio/src/app/custom-elements/getting-started/container/container.component.spec.ts
Outdated
Show resolved
Hide resolved
aio/src/app/custom-elements/getting-started/container/container.component.spec.ts
Outdated
Show resolved
Hide resolved
}); | ||
|
||
it('should create', () => { | ||
expect(component).toBeTruthy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
aio/src/app/custom-elements/getting-started/event-binding/event-binding.component.ts
Show resolved
Hide resolved
aio/src/app/custom-elements/getting-started/ng-for/ng-for.component.ts
Outdated
Show resolved
Hide resolved
aio/src/app/custom-elements/getting-started/property-binding/property-binding.module.ts
Outdated
Show resolved
Hide resolved
aio/src/app/custom-elements/getting-started/ng-if/ng-if.module.ts
Outdated
Show resolved
Hide resolved
2b8eaa9
to
9f76b82
Compare
You can preview 9f76b82 at https://pr26059-9f76b82.ngbuilds.io/. |
You can preview a4abdcf at https://pr26059-a4abdcf.ngbuilds.io/. |
a4abdcf
to
fb59be2
Compare
You can preview fb59be2 at https://pr26059-fb59be2.ngbuilds.io/. |
You can preview 9f50303 at https://pr26059-9f50303.ngbuilds.io/. |
34bddba
to
5db11b0
Compare
@gkalpak I've address your feedback with the observables and unnecessary tests. I didn't do anything with deriving the template from the results. I'll have to get some guidance add follow that with another PR. I'd like to get this landed first for Stephen, so we can iterate on the other getting started guide. |
@gkalpak You can see the widgets on the |
You can preview 34bddba at https://pr26059-34bddba.ngbuilds.io/. |
You can preview 5db11b0 at https://pr26059-5db11b0.ngbuilds.io/. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I would consider changing all widget selectors to ...-gs-...
(instead of ...-getting-started-...
for consistency. (The directory name should be a strong enough hint as to what gs
stands for.)
aio/src/app/custom-elements/getting-started/event-binding/event-binding.component.ts
Outdated
Show resolved
Hide resolved
aio/src/app/custom-elements/getting-started/interpolation/interpolation.component.ts
Outdated
Show resolved
Hide resolved
aio/src/app/custom-elements/getting-started/ng-for/ng-for.component.ts
Outdated
Show resolved
Hide resolved
aio/src/app/custom-elements/getting-started/ng-for/ng-for.component.ts
Outdated
Show resolved
Hide resolved
aio/src/app/custom-elements/getting-started/ng-for/ng-for.component.ts
Outdated
Show resolved
Hide resolved
aio/src/app/app.module.ts
Outdated
@@ -118,6 +119,7 @@ export const svgIconProviders = [ | |||
MatProgressBarModule, | |||
MatSidenavModule, | |||
MatToolbarModule, | |||
OverlayModule, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its needed for the tooltip that displayed when the JSON in invalid. I believe Angular Material needs to be at least version 6.1.x
to lazy load the OverlayModule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, importing MatTooltipModule
is not enough? That's weird. Is it because we lazy-load MatTooltipModule
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not enough to import MatTooltipModule
. It throws an error with the Overlay
provider being missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misunderstood. Yes its because MatTooltipModule
is lazy loaded.
aio/src/app/custom-elements/getting-started/ng-if/ng-if.component.ts
Outdated
Show resolved
Hide resolved
BTW, you can add the widgets in test.html (for showcase/review purposes). |
You can preview 536d2fa at https://pr26059-536d2fa.ngbuilds.io/. |
536d2fa
to
29fcf4c
Compare
You can preview 29fcf4c at https://pr26059-29fcf4c.ngbuilds.io/. |
d98e78e
to
f25f968
Compare
You can preview f25f968 at https://pr26059-f25f968.ngbuilds.io/. |
f25f968
to
de01285
Compare
You can preview de01285 at https://pr26059-de01285.ngbuilds.io/. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good now on mobile. thanks!
I'm not happy about the size in increase, but we'll investigate that more once we switch to ivy.
Can you please:
- rebase on master
- squash into a single commit
- prefix the commit with
feat(docs-infra):
("docs(docs-infra)" means documentation for docs-infrastruture).
thanks!
de01285
to
62244a8
Compare
62244a8
to
4f2014d
Compare
You can preview 4f2014d at https://pr26059-4f2014d.ngbuilds.io/. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Small interactive widget components for getting started