-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Unsupported browser (feature) #1252
Conversation
* | ||
* @type {Object} | ||
*/ | ||
let currentRoute = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
No global state please! Either React Component state or Redux store.
-
AbstractApp's
state
already tracks the current route. It has aroute
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed global state.
|
||
if (room !== oldRoom) { | ||
_navigate(state); | ||
if (!areRoutesEqual(newRoute, currentRoute)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RouteRegistry
clones routes before it exposes them from its internal state so areRoutesEqual
would probably better fit as a (static?) method of RouteRegistry
.
dispatch(setRoom(newRoom)); | ||
|
||
const state = getState(); | ||
const room = state['features/base/conference'].room; | ||
const { app } = state['features/app']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source code from this line to the end of the method would probably better fit in App
's _navigate
method.
* | ||
* @class Landing | ||
*/ | ||
class Landing extends Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace the word landing in all its occurrences (class, Redux action, Redux state, etc.) with something more appropriate given that the feature is named unsupported-browser
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed Landing
component and logic related to that to MobileBrowserPage
const link = typeof room === 'undefined' | ||
&& typeof domain === 'undefined' | ||
? urlOrRoom | ||
: room; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatting of the above is not in accord with the existing source code.
// they will stay unchanged in the registry. | ||
const route = { ...RouteRegistry.getRouteByComponent(component) }; | ||
|
||
if ((OS === 'android' || OS === 'ios') && !landingIsShown) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check should be whether the execution environment supports the app. The biggest piece of it is probably whether WebRTC is supported.
Only then does come the refinement that we choose to promote the mobile app in Google Chrome (if it supports WebRTC) on Android.
c0f4460
to
2f0d786
Compare
React Native provides a Platform abstraction which React does not provide.
2f0d786
to
0051b3b
Compare
No description provided.