search

askvortsov1 / flarum-pwa

Progressive Web App support for Flarum
MIT License
34 stars 12 forks source link

Add support for Window Controls Overlay #53

Closed DellZHackintosh closed 7 months ago

DellZHackintosh commented 10 months ago

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_override item 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.

DellZHackintosh commented 7 months ago

It would be easier for a maintainer to understand the proposed changes if you clarified why it is needed.

It just influences the appearance of the PWA and it even not supported by all browsers. But as I have explained, it's in line with the current goal (To do: Support configuration of ALL webmanifest attributes), so I made this.

DellZHackintosh commented 7 months ago

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. Mark it as "advanced" and the links I have provided maybe will do help, but it seems that I should think a clearer description text... How about this one?

          If true, users will be able to get an more immersive, native-like experience by hiding the native titlebar. Window Controls Overlay enables or not depends on user's preference and browser's compatibility. You must customize your titlebar by adding some CSS/JS after enabled, or the controls will overlay some elements of your forum's top navbar and the window almost can't drag, resulting a bad experience.
askvortsov1 commented 7 months ago

How do you feel about the wording change I proposed in the review comments?

DellZHackintosh commented 7 months ago

It is indeed easier to understand, although the description of the feature can be misleading.

askvortsov1 commented 7 months ago

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

DellZHackintosh commented 7 months ago

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: The Button of disabling the feature With this button, users can switch the feature. So that's why this feature should depend on users. By the way, the feature is disabled until users press that button. 😂

askvortsov1 commented 7 months ago

Ah. Right. Yeah that's completely valid, totally missed it, sorry. Your wording sounds good then!

DellZHackintosh commented 7 months ago

Well, it's all right now.

DellZHackintosh commented 7 months ago

@askvortsov1 The recommendations have been implemented.

DellZHackintosh commented 7 months ago

Thanks very much! I'll keep on adding more features to it. 😊