Add support for Window Controls Overlay #53
Conversation
There was a problem hiding this comment.
It would be easier for a maintainer to understand the proposed changes if you clarified why it is needed. Adding screenshots, related articles and a wider motivation to the PR would be really helpful.
I've left some things as a review that seem to need improvement. As I'm not a maintainer of this package, I cannot guarantee the PR to merged regardless of further effort to make the changes I mentioned.
resources/locale/en.yml
Outdated
| @@ -32,6 +32,12 @@ askvortsov-pwa: | |||
| push_notif_preference_default_to_email_text: If true, users will have push notifications enabled by default for all notification types where email notifications are enabled by default. If false, users will have push notifications disabled by default. | |||
| user_max_subscriptions_label: Max Push Subscriptions Per User | |||
| user_max_subscriptions_text: How many push subscriptions should each user be able to have at a time? When a user receives a push notification, each subscription will result in an API call. Each subscription typically corresponds to a browser the user has used. | |||
| windowControlsOverlay_label: Enable Window Controls Overlay? | |||
| windowControlsOverlay_text: | | |||
| If true, users will able to enable Window Controls Overlay to create an immersive, native-like experience. Window Controls Overlay enables or not depends on user's preference and browser's compatibility. You also should be sure you've done something to match this feature. | |||
There was a problem hiding this comment.
You should be sure you've done something to match this feature.
What does that even mean or imply?
There was a problem hiding this comment.
Sorry, it may not be described clearly. As the text mentioned, we must do something to adapt the UI changes.
Specifically speaking, after enabling this feature, the titlebar will disappear and only leave the three controls. Flarum doesn't do anything for that, so you'll have a bad window. For example:
Just like that. So you're needed to do is just write some CSS to "match this feature". For example (Please pay attention to the position changes of the elements on the titlebar):
That's all. Since the description text is considered as incomprehensible, maybe it's necessary to rewrite it......If someone can provide help, I would be grateful.
Fix misspelled variable Co-authored-by: Daniël Klabbers <[email protected]>
It just influences the appearance of the PWA and it even not supported by all browsers. |
There was a problem hiding this comment.
Overall lgtm, sorry for not getting to this earlier! Left a few wording / style nits.
I do worry slightly that the need to provide custom CSS / JS in addition to enabling the feature will lead to confusion for less technical forum admins; how would you feel about labelling this as an "advanced" setting"?
|
No problem! For the advice, yes, the feature itself and my description text will do make the admins confused. The feature itself doesn't have the ability to add any code to adapt the overlayed controls, it just hide the native titlebar, even also can't change its size, position or other though it. Besides, the CSS/JS code can only be added by other possible way. |
|
How do you feel about the wording change I proposed in the review comments? |
|
It is indeed easier to understand, although the description of the feature can be misleading. |
|
Fair enough. I'm happy with your newest proposed wording, except for the "users will be able to" part, because this is something configured per-forum, not per-user |
No. Please pay attention to it: |
|
Ah. Right. Yeah that's completely valid, totally missed it, sorry. Your wording sounds good then! |
|
Well, it's all right now. |
Co-authored-by: Alexander Skvortsov <[email protected]>
Co-authored-by: Alexander Skvortsov <[email protected]>
|
@askvortsov1 The recommendations have been implemented. |
|
Thanks very much! I'll keep on adding more features to it. 😊 |
Originally mentioned in Flarum Discuss, and this pull aims to add that support.
Changed 3 files in order to add the setting and write the
display_overrideitem into the manifest.Though there's few supports for Window Controls Overlay for Flarum (maybe there's only some of my attempts), it's in line with the current goal.