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

Status bar #850

Closed
wants to merge 6 commits into from
Closed

Status bar #850

wants to merge 6 commits into from

Conversation

piranna
Copy link

@piranna piranna commented Mar 4, 2018

Implement StatusBar component instead of being a noop. This allow to set backgroundColor as navigation bar color in Chrome for Android, and all the other props and static functions with their iOS equivalents by injecting and updating their meta tags.

import { Component } from 'react';


const {head} = document
Copy link
Owner

Choose a reason for hiding this comment

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

This needs to be able to run in a server-side context too. Use canUseDOM guard as done elsewhere

Copy link
Author

Choose a reason for hiding this comment

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

Didn't know of that, fixed.



function getMetaTag(attrName) {
const htmlCollection = head.getElementsByTagName('meta')
Copy link
Owner

Choose a reason for hiding this comment

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

No need to collect all the meta tags when all you need is something like

document.querySelector(`meta[name=${attrName}]`)

Copy link
Author

Choose a reason for hiding this comment

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

Good trick, thank you! :-) Fixed.

}

function setAppleMobileWebAppCapable() {
getMetaTag('apple-mobile-web-app-capable').content =
Copy link
Owner

Choose a reason for hiding this comment

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

Setting apple-mobile-web-app-capable is problematic (it's basically a broken feature), which is why most web apps do not set it.

Copy link
Author

Choose a reason for hiding this comment

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

I've used it because seems Apple docs say it's needed to enable the apple-mobile-web-app-status-bar-style meta tag and make it works. Do you know of any other alternative?

static setBarStyle() {}
static setHidden() {}
static get currentHeight() {
const {availHeight, height} = screen
Copy link
Owner

Choose a reason for hiding this comment

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

window.screen

Copy link
Author

Choose a reason for hiding this comment

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

Done.

static setBarStyle(style, animated) {
if(style === 'light-content') style = 'black'

if(!['black', 'black-translucent', 'default'].includes(style)) return
Copy link
Owner

Choose a reason for hiding this comment

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

includes is not polyfilled and it's not necessary to create an array to perform this check

Copy link
Author

Choose a reason for hiding this comment

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

I'm used to use includes(), but it's ok for me to change that. Done.

@necolas
Copy link
Owner

necolas commented Mar 5, 2018

You need to run yarn test and fix the errors

@piranna
Copy link
Author

piranna commented Mar 13, 2018

You need to run yarn test and fix the errors

Fixed Flow errors. There are some remaining ones, but are related to other components.

Copy link
Owner

@necolas necolas left a comment

Choose a reason for hiding this comment

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

I'm not sure that all of this is particularly useful. Perhaps only keep the theme color for apps not using a service worker / manifest or wanting to customize the tab bar color for mobile chrome.

The Apple web app stuff is broken in Safari. And the translucent setting in native is Android only, but here it's iOS Safari only. I think that can be removed.

@piranna
Copy link
Author

piranna commented Mar 16, 2018

I'm not sure that all of this is particularly useful. Perhaps only keep the theme color for apps not using a service worker / manifest or wanting to customize the tab bar color for mobile chrome.

I only needed the color theme just for aesthetics, since I knew that Chrome supported the change of the navigation bar it I though it could be a good replacement to the Android and iOS support to change status bar color. I tried to implement the other things just to don't have the API incomplete.

The Apple web app stuff is broken in Safari

Lol...

And the translucent setting in native is Android only, but here it's iOS Safari only. I think that can be removed.

Platform dependent options are noop for the ones that doesn't have an alternative, but if we can have some way to implement some behaviour available in just some of the platforms instead of all of them, I don't see why we shouldn't add it as far as the API is the same... In fact, Picker don't support dialog mode on iOS and I have implemented it in a module with the same API (sort of polyfill, if you want to see it that way), and I'm thinking to do a pull-request on ReactNative code to add that...

@necolas
Copy link
Owner

necolas commented Apr 2, 2018

Thanks for this, but at this stage I'm going to close it for the reasons previously mentioned.

@necolas necolas closed this Apr 2, 2018
@piranna
Copy link
Author

piranna commented Apr 3, 2018

It's your project and your decission, but it's not fair. I've investigated about how to implement this and filled all the possible behaviour, and also fixed the problems that you showed up. Maybe it's not too much useful, but improve apps eyecandy and more important implement a missing API, I think this is enough reason to accept it.

@necolas
Copy link
Owner

necolas commented Apr 3, 2018

You didn't respond to requests for further changes, and unfortunately Safari's web app features currently have bugs that you can't do anything about. These settings are meant to be defined in an app manifest file on web now, that's the expectation for PWAs.

piranna added a commit to piranna/react-native-web-statusbar that referenced this pull request May 10, 2018
@necolas necolas mentioned this pull request Apr 13, 2021
@shamilovtim
Copy link

You've mentioned using a PWA manifest -- I assume that's for mobile safari and mobile chrome. What I'm not seeing is anything about an identical looking purely front end polyfill for desktop browsers. Philosophically, if we can make a button that looks like a button, or other widgets that look on web like they look on mobile, I don't see why we can't have a status bar that looks like a mobile statusbar, with the handful of customization that the mobile one has (mostly color). I don't know if this user's solution is correct, I just feel like this polyfill is missing.

@necolas
Copy link
Owner

necolas commented Apr 13, 2021

There's no value in creating a fake status bar for desktop. No apps want or need that. Apps should feel native to their host environment

Repository owner locked as resolved and limited conversation to collaborators Apr 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants