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

[WD-11261] Implementation of LXD PWA with a dynamic start_url #785

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

Kxiru
Copy link
Contributor

@Kxiru Kxiru commented May 28, 2024

Done

  • Implemented minimal PWA support for the LXD-UI client in index.html.
    • The manifest file/variable supports a dynamic start_url link.
  • Added a Logo image for the minimum requirements of the PWA.
  • Added a LXD-Screenshot image for the minimum requirements of the PWA.

Specification: LX078 - PWA Support

QA

  1. Run the LXD-UI:
    • On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @mas-who or @edlerd for access.
    • With a local copy of this branch, run as described here.
  2. Perform the following QA steps:
    • Open the LXD-UI Client and note the PWA pop up, or, view the
      image
      --icon in the url toolbar. One can click this to trigger the installation sequence.

Screenshots

image
image
image

@webteam-app
Copy link

@Kxiru Kxiru marked this pull request as draft May 28, 2024 14:01
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Thanks for putting this up!

Some comments below to make it work on the demo server.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
@mas-who
Copy link
Contributor

mas-who commented May 30, 2024

@Kxiru I think David has covered all the points. After making the changes for the screenshot reference path, it should work on the demo server as well.

In case if you are wondering why the path should be changed, you can go to the Source tab in your chrome inspector (see below for reference)

Screenshot from 2024-05-30 09-57-49

Edit: Can you also ask @piperdeck to do a test on Safari for this.

Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

The paths to the assets need to be prefixed with /ui and the scope field needs fixing.

I am not sure the orange menu bar is the best option, it seems a rather busy colour for it. Maybe use the grey/black from the left-hand navigation, or consult with design for this.

index.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

It works on localhost, but not yet on the demo server -- and I fear it won't work in the real build either. See comments below.

index.html Outdated Show resolved Hide resolved
manifest.js Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
@Kxiru Kxiru force-pushed the PWA-support branch 3 times, most recently from 635dd11 to fbe04bd Compare June 4, 2024 21:24
@Kxiru
Copy link
Contributor Author

Kxiru commented Jun 4, 2024

@edlerd , I've got it to work on the demo server and I've tidied up index.html a bit too :).

@Kxiru Kxiru requested a review from edlerd June 4, 2024 21:29
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

This works great on the demo server with signed certificates!
I found some small improvements that should be done before merging it.

public/assets/js/manifest.js Outdated Show resolved Hide resolved
public/assets/js/manifest.js Outdated Show resolved Hide resolved
public/assets/img/LXD-screenshot2.png Outdated Show resolved Hide resolved
public/assets/js/manifest.js Outdated Show resolved Hide resolved
- Added Screenshot images to /public/assets/img/ for use in teh manifest variable in index.html.

Signed-off-by: Nkeiruka <nkeiruka.whenu@canonical.com>
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for driving this forward.

@Kxiru Kxiru marked this pull request as ready for review June 5, 2024 15:49
@Kxiru Kxiru merged commit 0e5e934 into canonical:main Jun 5, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 5, 2024
@Kxiru Kxiru deleted the PWA-support branch June 17, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants