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

[Issue-3666] Add PWA to docusaurus #3693

Merged
merged 1 commit into from
Oct 12, 2022
Merged

Conversation

Kevan-Y
Copy link
Contributor

@Kevan-Y Kevan-Y commented Oct 7, 2022

Issue This PR Addresses

Fixes #3666

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

  • Add PWA to docusaurus

Steps to test the PR

  • Checkout branch
  • Run pnpm i
  • Run pnpm --filter @senecacdot/telescope-docs build && pnpm --filter @senecacdot/telescope-docs serve

Should able to see this button to use PWA
image

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@Kevan-Y Kevan-Y added hacktoberfest-accepted area: docusaurus Anything related to Docusaurus labels Oct 7, 2022
@Kevan-Y Kevan-Y self-assigned this Oct 7, 2022
@gitpod-io
Copy link

gitpod-io bot commented Oct 7, 2022

Comment on lines +4 to +5
"theme_color": "#121D59",
"background_color": "#FFFFFF",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to make use of the theme colours defined here

--ifm-color-primary: #121d59;

--ifm-color-primary-lightest: #ffffff;

Copy link
Contributor

@cindyorangis cindyorangis Oct 7, 2022

Choose a reason for hiding this comment

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

I think we can address this in another issue. I've tried setting "theme_color": "var(--ifm-color-primary)" but Chrome dev tools said it was invalid and automatically set the colour to white and I don't want to make this PR bigger than it already is.

2022-10-07 13_39_55-Window

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be

"theme_color": var(--ifm-color-primary)

without the quotes around var()
but, I am quite weak at front-end so I'll let the front-end gurus decide what to do

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I initially thought, too, but apparently

"theme_color": var(--ifm-color-primary)

is not valid JSON... this can totally be someone else's issue ;D

Copy link
Contributor Author

@Kevan-Y Kevan-Y Oct 7, 2022

Choose a reason for hiding this comment

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

@humphd
Copy link
Contributor

humphd commented Oct 7, 2022

TIL about pnpm --filter. Question: could we use this to improve what we do in pnpm dev here? cc @cindyledev

@cindyorangis
Copy link
Contributor

TIL about pnpm --filter. Question: could we use this to improve what we do in pnpm dev here? cc @cindyledev

Wow, I didn't know pnpm --filter was a thing! It works for pnpm dev! I'll create a PR. Thank you based @Kevan-Y

cindyorangis
cindyorangis previously approved these changes Oct 7, 2022
Copy link
Contributor

@cindyorangis cindyorangis left a comment

Choose a reason for hiding this comment

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

Works on my machine. Very cool.

@RC-Lee
Copy link
Contributor

RC-Lee commented Oct 7, 2022

Running into this

PS C:\OSD\telescope> pnpm --filter @senecacdot/telescope-docs build

> @senecacdot/[email protected] build C:\OSD\telescope\src\web\docusaurus
> docusaurus build

[ERROR] Minimum Node.js version not met :(
[INFO] You are using Node.js v16.13.0, Requirement: Node.js >=16.14.
C:\OSD\telescope\src\web\docusaurus:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  @senecacdot/[email protected] build: `docusaurus build`
Exit status 1

@Kevan-Y
Copy link
Contributor Author

Kevan-Y commented Oct 7, 2022

Running into this

PS C:\OSD\telescope> pnpm --filter @senecacdot/telescope-docs build

> @senecacdot/[email protected] build C:\OSD\telescope\src\web\docusaurus
> docusaurus build

[ERROR] Minimum Node.js version not met :(
[INFO] You are using Node.js v16.13.0, Requirement: Node.js >=16.14.
C:\OSD\telescope\src\web\docusaurus:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  @senecacdot/[email protected] build: `docusaurus build`
Exit status 1

What is your node version?

@RC-Lee
Copy link
Contributor

RC-Lee commented Oct 7, 2022

Running into this

PS C:\OSD\telescope> pnpm --filter @senecacdot/telescope-docs build

> @senecacdot/[email protected] build C:\OSD\telescope\src\web\docusaurus
> docusaurus build

[ERROR] Minimum Node.js version not met :(
[INFO] You are using Node.js v16.13.0, Requirement: Node.js >=16.14.
C:\OSD\telescope\src\web\docusaurus:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  @senecacdot/[email protected] build: `docusaurus build`
Exit status 1

What is your node version?

Right now I'm trying to build the containers in docker to see if it works.
But think its worth noting to see how it might clash with our node version in package.json

"node": ">=14.0.0",

@RC-Lee
Copy link
Contributor

RC-Lee commented Oct 7, 2022

I'm a little confused with PWA and what it does.
Is there a place I'm supposed to toggle it on?
image

@Kevan-Y
Copy link
Contributor Author

Kevan-Y commented Oct 7, 2022

Is there a place I'm supposed to toggle it on?

It suppose to have but running on dev mode it won't be shown

@RC-Lee
Copy link
Contributor

RC-Lee commented Oct 7, 2022

Is there a place I'm supposed to toggle it on?

It suppose to have but running on dev mode it won't be shown

You might want some more experienced (with docusaurus and front-end) reviewers to review this, since this is out of my comfort zone.
I can go the easy way and update my node version, but once again I don't know how that might conflict with our package.json

"node": ">=14.0.0",

And since its hidden in dev mode, I can't see if its working through docker containers locally

@Kevan-Y
Copy link
Contributor Author

Kevan-Y commented Oct 7, 2022

@RC-Lee I believe your browser is running under Firefox. Use a Chromium-based browser

Note: Currently, only the Chromium-based browsers such as Chrome, Edge, and Samsung Internet require the service worker. If developing your app using Firefox, be aware that you will need a service worker to be compatible with Chromium-based browsers.
see https://developer.mozilla.org/en-US/docs/Web/Progressive_web_apps/Installable_PWAs#requirements

@RC-Lee RC-Lee removed their request for review October 7, 2022 20:15
@Kevan-Y
Copy link
Contributor Author

Kevan-Y commented Oct 7, 2022

@humphd @cindyledev @RC-Lee Wondering if it would be good to have a button to install app, so it would be more accessible for people that doesn't know what is PWA.

image

@humphd
Copy link
Contributor

humphd commented Oct 8, 2022

Having a link to do it makes sense I think, yes.

@Kevan-Y
Copy link
Contributor Author

Kevan-Y commented Oct 8, 2022

@humphd Added

humphd
humphd previously approved these changes Oct 11, 2022
@Kevan-Y
Copy link
Contributor Author

Kevan-Y commented Oct 12, 2022

@cindyledev Just need a re-approval, then I will squash those commit and merge

@humphd
Copy link
Contributor

humphd commented Oct 12, 2022

Why are we merging master into this branch?

Screen Shot 2022-10-12 at 9 44 20 AM

@Kevan-Y
Copy link
Contributor Author

Kevan-Y commented Oct 12, 2022

Why are we merging master into this branch?

Screen Shot 2022-10-12 at 9 44 20 AM

So I could use Squash and the merge button from GitHub.
image
I needed to make sure my branch have the most updated master commit.

@humphd
Copy link
Contributor

humphd commented Oct 12, 2022

I don't care how you want to do it so long as we don't end-up with master merged into topic branches. Ideally you'd squash on your own and merge that.

@Kevan-Y
Copy link
Contributor Author

Kevan-Y commented Oct 12, 2022

I don't care how you want to do it so long as we don't end-up with master merged into topic branches. Ideally you'd squash on your own and merge that.

Alright, I rebased/Squash manually

Copy link
Contributor

@cindyorangis cindyorangis left a comment

Choose a reason for hiding this comment

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

Pretty slick, if I do say so myself

2022-10-12 13_01_30-Telescope Documentation - Environment Setup _ Telescope

@Kevan-Y Kevan-Y merged commit 4e8648f into Seneca-CDOT:master Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: docusaurus Anything related to Docusaurus hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PWA to docusaurus
4 participants