Skip to content

feat: Angular boilerplate #174

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

Merged
merged 8 commits into from
May 7, 2024
Merged

Conversation

9kubczas4
Copy link
Contributor

@9kubczas4 9kubczas4 commented May 3, 2024

PR includes a basic Angular Hello World boilerplate for Firebase App Hosting.

It shows basic features like:

  • SSR
  • SSG
  • Deferrable Views

</article>
</section>

<section class="links-container">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jhuleatt @jamesdaniels please let me know what links should be displayed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW the links were removed in the Next.js boilerplate

Copy link
Collaborator

Choose a reason for hiding this comment

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

let's remove the links, but keep the CSS please so we can easily add them back in later.

@9kubczas4 9kubczas4 requested a review from jhuleatt May 3, 2024 16:59
Copy link
Collaborator

@jhuleatt jhuleatt left a comment

Choose a reason for hiding this comment

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

Looks good to me once links are removed (#174 (comment)).

Seems like CI is failing, is that an easy fix?

@Yuangwang
Copy link
Collaborator

Could you add a brief description of what this is for?

@9kubczas4
Copy link
Contributor Author

@Yuangwang just added a short description to the PR.

Copy link
Collaborator

@Yuangwang Yuangwang left a comment

Choose a reason for hiding this comment

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

Two small comments, but otherwise LGTM once the comments are addressed and CI is fixed

@@ -0,0 +1,22 @@
<h1 class="large-heading">Hello, My App</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be changed to something else?

I think the nextjs template calls this page "Next.js on Firebase App Hosting" so maybe "Angular on Firebase App Hosting" works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,

It was implemented based on designs.
I'm more than happy to introduce some content changes, but I just wanted to be sure that all of us are on the same page.

CC: @jhuleatt

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to mirror the Next.js h1

<h1 className="heading">Next.js on Firebase App Hosting</h1>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've applied proposal in recent commit.

@@ -0,0 +1,3 @@
<h1 class="heading">Sample title for SSG</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should probably be renamed, maybe just SSG is good

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed the Next.js one to just SSG so I think this one can follow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've applied proposal in recent commit.

@leoortizz
Copy link
Contributor

updated this PR to get the package lock fix from #173

@9kubczas4 9kubczas4 requested a review from leoortizz May 6, 2024 17:24
@9kubczas4
Copy link
Contributor Author

Hi @Yuangwang,

The build step is failing on CI, locally it works fine.

The following error occurs:

Error: src/firebase-aware.ts(8,17): error TS7016: Could not find a declaration file for module 'lru-cache'. '/home/runner/work/firebase-framework-tools/firebase-framework-tools/node_modules/lru-cache/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/lru-cache` if it exists or add a new declaration (.d.ts) file containing `declare module 'lru-cache';`
Error: src/firebase-aware.ts(23,13): error TS7006: Parameter 'value' implicitly has an 'any' type.
npm ERR! Lifecycle script `build` failed with error: 
npm ERR! Error: command failed 
npm ERR!   in workspace: firebase-frameworks@0.11.2 
npm ERR!   at location: /home/runner/work/firebase-framework-tools/firebase-framework-tools/packages/firebase-frameworks 

It looks like it's unrelated to changes in this PR.

9kubczas4 and others added 2 commits May 6, 2024 19:45
Co-authored-by: Leonardo Ortiz <leo@monogram.io>
Co-authored-by: Leonardo Ortiz <leo@monogram.io>
@9kubczas4 9kubczas4 requested a review from leoortizz May 6, 2024 17:46
@Yuangwang Yuangwang merged commit a49debd into main May 7, 2024
7 of 8 checks passed
@jamesdaniels jamesdaniels deleted the pawelkubiak_angular-boilerplate branch May 7, 2024 18:06
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.

5 participants