Basic PWA support

[GamerClassN7]

Hello,
I am so happy i can at least try to contribute to this awesome peace of software :)

my humble contribution is to fix/tweak ease of use on mobile phones :)

my MR add Basic PWA support to convert lack off Mobile and desktop applications for bookstack. I have more knowledge in implementing PWA support to laravel project so if you would be open to discussion i can try to help more in this spectrum :)

Screenshots

[GamerClassN7]

Hmm, could someone suggest where to move manifest definition array so 'codeclimate' is happy ?
Thank you in advance!

[ssddanbrown]

Thanks for offering this @GamerClassN7!
Before getting into the technical/code side of things, could you just confirm the actual functional goals/intent of this PR?
It's been explained as adding PWA support, but that's really an implementation detail. What are the key targeted goals & user-facing functional intent of the changes here?

[GamerClassN7]

Hello @ssddanbrown,
I administrate several instances of BS, and only think users are complaining about is lack off Android/IOS Applications, so i implemented this changes (my PR) to add first step in PWA architecture -> manifest.json which enable for end user download page as normal app, search it in theirs os menus etc. It is not full PWA implementation. (No offline support ETC.) But for most of end users with acces to stable at least 3D it is sufficient replacement of OS native APP. I am using it for more of a year, and after each update i need to reapply my changes so i decided, to try my luck and create PR.... I think at least to my research and Reddit threads that biggest handicap of BS is lack of APP that is why :)

regarding the technical implementation, i jus use Laravel router to emulate manifest.json file and add proper colors and names from application settings :)

BR
J.

[ssddanbrown]

Okay, so to summarize this is specifically targeted at making it easier for users to add to their mobile devices for an app-like experience.

In regard to the technical side, I'll probably give this a review in the later part of the release cycle (later this month) for initial feedback.

Thanks again!

[GamerClassN7]

@ssddanbrown will be WATING for your feedback :) feel free to contact me any time.

And thank you once more for this amazing OS Software :)

[ssddanbrown]

@Daveyvdweide Thanks for offering a review but I'm going to resolve your comments since I don't want @GamerClassN7 to needlessly spend time on addressing these points. Syntax and formatting can be picked up by our tooling, and fixed when I merge in the code. In regards to the where formatting, I prefer the style as originally written (and I do the same throughout the BookStack codebase so it's consistent). Additionally, minor details may be redundant to address if there are wider implementation changes to be made.

[ssddanbrown]

Okay, Sorry for the delay again @GamerClassN7, ended up being after the last release cycle as time got a bit more limited.

I've now given this a play in my dev environment. It did indeed change the behaviour of using "Add to homescreen" on both iOS/Safari and Chrome/Android, to be a more "contained" app-like experience, so that's working well!

In regard to the code, looks like a good start but a few things that need to be amended:

• Please could the formatting/whitespace changes be removed from HomeController? Currently shows a lot more changes than this PR relates to, and moves the formatting away fro expected project formatting. Ideally these changes would be removed from any commits here. Easiest way might be to commit to change whitespace back, then squash your commits, then force-push back up to the branch. I can squash on my side if easier but it does essentially re-write commits which can affect attribution.
• Could the manifest array be moved to a new PwaManifestBuilder class or similar, since it's not really app config itself, and we can avoid re-using env calls while allowing more complex logic (which we'll need for the next item).
  • As part of this, replace env calls to config calls. This should also avoid the need to using default/backup values.
• In regards to manifest details:
  • Since BookStack v23.01 we've supported the generation of custom app icon files. We'd need to support them in this for the various icon sizes. You can copy the usage method from their use in our base template file here.
  • Probably best to remove short_name if allowed, rather than leaving in a default that may not at all be familiar to end users.
  • I think we should maybe avoid categories for now since they're experimental, and it's hard to categorise a platform that may be used for many different purposes. Think it'd be better to wait until things are more standardised/supported and there's practical reasons to implement, rather than add now.
  • Would probably be nice to add the theme_color into this, since we have relatively safe/easy access to it via setting('app-color').
  • For background_color, might be slightly better to use #F2F2F2 to match the light mode BG color, for a smoother transition for at least one of the color schemes.
  • Other values all look good as sensible defaults.
• Ideally we'd have testing added to cover this functionality, as per our existing range of PHPUnit tests.

Hope that all makes sense and is reasonable. Are you happy to make those changes?
Feel free to ask for help or guidance if needed, or feel free to query any of the above if desired. If you're not keen to take this further, or perform any of the above yourself, then just let me know and I can decide a path forward.

Thanks again for offering this PR to make a start on this.

[GamerClassN7]

Hello @ssddanbrown 👋 thank you for your feedback. I will try to read it tomorrow in more detail.

Could you before hand specifically where I should put the New PWA class you are mentioning ?

[ssddanbrown]

@GamerClassN7 Just throw it in the app/App folder for now, since that's where the HomeController is too. Can always re-arrange in the future when/if there's a better place for it.

[GamerClassN7]

@ssddanbrown Hi, Finally had some time :)

I would need little help with followings:

1. add support for icons in dimension 48x48 for Windows part of PWA
2. I am not too familiar with Native PHP unit tests i use dusk most of the time.

BR
Jonatan

[ssddanbrown]

Thanks @GamerClassN7!

add support for icons in dimension 48x48 for Windows part of PWA

Are those a suggestion or a requirement for PWAs on windows? What happens without 48px icons? I'd prefer to leave those out as long as things gracefully fall back rather than complicate anything our side to suit Windows suggestions.

I am not too familiar with Native PHP unit tests i use dusk most of the time.

Okay, would you like me to take those on or are you looking to learn that under some guidance?
If it helps, I very recently expanded the documentation on our PHP testing to provide a lot more detail and guidance.

[GamerClassN7]

Thanks @GamerClassN7!

add support for icons in dimension 48x48 for Windows part of PWA

Are those a suggestion or a requirement for PWAs on windows? What happens without 48px icons? I'd prefer to leave those out as long as things gracefully fall back rather than complicate anything our side to suit Windows suggestions.

I am not too familiar with Native PHP unit tests i use dusk most of the time.

Okay, would you like me to take those on or are you looking to learn that under some guidance? If it helps, I very recently expanded the documentation on our PHP testing to provide a lot more detail and guidance.

Thays are in guidelines for PWA, shouldn't be too hard right since you are already makings all of those icons.. (Manifest is not compliant without them)
I would love to learn them but dont have time IRL, So if you could be so kind and make them please ?

[GamerClassN7]

@ssddanbrown any news ? :)

[ssddanbrown]

@GamerClassN7 Not really. Sorry, I tend to bundle PRs and go through them on certain days.

Not had a look at the changes yet, but I'm happy to take on the tests to finish this off.
I'm not going to worry about the extra icon size unless there's a proven functional need for it, things get ridiculous if we follow guidelines for all platforms, would rather keep things simpler unless based on functional purpose.

[GamerClassN7]

@GamerClassN7 Not really. Sorry, I tend to bundle PRs and go through them on certain days.

Not had a look at the changes yet, but I'm happy to take on the tests to finish this off.
I'm not going to worry about the extra icon size unless there's a proven functional need for it, things get ridiculous if we follow guidelines for all platforms, would rather keep things simpler unless based on functional purpose.

Hi @ssddanbrown 👋,
They are PWA guidelines not platform specific, manifest generally need at least one mentioned size in som "mascable format" I sticker to favicon for this time until better solution become available, to be able go threw PWA check with all stars :) :) hope you understand :)

[ssddanbrown]

Thanks again @GamerClassN7, now merged into dev to be part of the next feature release.
Tests added, and some minor tweaks made, in 1d91b4d.

They are PWA guidelines not platform specific [...] to be able go threw PWA check with all stars

Not sure where those guidelines are coming from, but I couldn't see the size required in the spec so some circumstance as far as I'm concerned, not looking to make changes unless there's a functional need. I don't want to spend time chasing a certain companies guidelines or chasing full-stars/marks in an automated test.