Mobile landing #1213
Mobile landing #1213
Conversation
idainatovych
force-pushed
the
mobile-landing
branch
from
75f201e to
56fedd6
Compare
lyubomir
force-pushed
the
mobile-landing
branch
from
4b365a4 to
f9b13e5
Compare
| /** | ||
| * Flag that shows that mobile landing shown shown. | ||
| * | ||
| * @type {App} |
There was a problem hiding this comment.
The type of the property landingIsShown is definitely not App, it is boolean.
| ...state, | ||
|
|
||
| /** | ||
| * Flag that shows that mobile landing shown shown. |
There was a problem hiding this comment.
shown shown should be is shown.
| } | ||
| } | ||
|
|
||
| const mapStateToProps = state => { |
There was a problem hiding this comment.
A const with an arrow function as its value is not necessary here, a simple function definition is preferable.
| this.props.dispatch(landingIsShown()); | ||
| } | ||
|
|
||
| static propTypes = { |
There was a problem hiding this comment.
Static members go before instance members. Another way to look at it is, comply with the existing source code.
| import React, { Component } from 'react'; | ||
| import { connect } from 'react-redux'; | ||
| import { Link } from 'react-router'; | ||
| import { landingIsShown } from '../actions'; |
There was a problem hiding this comment.
Imports must be grouped by type.
| import { Link } from 'react-router'; | ||
| import { landingIsShown } from '../actions'; | ||
|
|
||
| const links = { |
There was a problem hiding this comment.
Constants have names with upper-case letters.
There was a problem hiding this comment.
We do require documentation (comments) on everything.
|
|
||
| const links = { | ||
| 'android': 'https://play.google.com/store/apps/details?id=org.jitsi.meet', | ||
| 'ios': '' |
There was a problem hiding this comment.
The iOS link is still missing.
| @@ -1,2 +1,3 @@ | |||
| export * from './loadScript'; | |||
| export * from './roomnameGenerator'; | |||
| export * from './detectDevices'; | |||
There was a problem hiding this comment.
The coding style requires sorting by alphabetical order.
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
That's too complicated. It can be improved by:
-
Returning the condition itself.
-
Matching against a single regular expression.
| @@ -50,6 +50,7 @@ export class App extends AbstractApp { | |||
| componentWillMount(...args) { | |||
| super.componentWillMount(...args); | |||
|
|
|||
| this.props.store.dispatch(detectPlatform()); | |||
There was a problem hiding this comment.
How many actions will we dispatch in App's componentWillMount. The super dispatches APP_WILL_MOUNT. In a previous PR you added appInit. And now we have detectPlatform. That's more than I'd like to have dispatched right here.
| * @returns {{type, platform: string}} | ||
| * @private | ||
| */ | ||
| export function _setPlatform(platform) { |
There was a problem hiding this comment.
The function is both private and exported? Is it feature internal?
| * been already detected. | ||
| * | ||
| * @param {string} platform - Mobile user agent platform. | ||
| * @returns {{type, platform: string}} |
There was a problem hiding this comment.
type what? It's type: APP_SET_PLATFORM.
| * platform: String | ||
| * } | ||
| */ | ||
| export const APP_SET_PLATFORM = Symbol('APP_SET_PLATFORM'); |
There was a problem hiding this comment.
Sort by alphabetical order.
| dispatch(_setPlatform('ios')); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
How does that relate to React Native's Platform class? You basically want us to use two separate abstractions called platform.
| /** | ||
| * Landing | ||
| */ |
There was a problem hiding this comment.
I already commented on a previous PR of yours about the above formatting of documentation comments but here it goes again: that's not the convention in the file and in the project.
| * | ||
| * @returns {boolean} | ||
| */ | ||
| export function detectIOS() { |
There was a problem hiding this comment.
Maybe using bowser could help here?
| // If landing was shown there is no need to show it again. | ||
| if (platform && !landingIsShown) { | ||
| component = detectAndroid() || detectIOS() ? Landing : component; | ||
| } |
There was a problem hiding this comment.
I find the above if more complex than necessary:
-
There's an assignment of
componenttocomponent. -
There're 2 separate conditions that may be merged into 1 condition.
-
The path that chooses the
Landingcomponent doesn't need the initialization of thecomponentvariable.
lyubomir
force-pushed
the
mobile-landing
branch
2 times, most recently
from
2322283 to
56fedd6
Compare
|
A showstopper for me in this PR is the fact that the location /mobile-app is treated as a room at the client. While at that location, a refresh will force the browser to query the server for /mobile-app and the server will respond with a 404. |
|
I've rebased this PR on the latest master (at the time of this writing), I've refactored it to comply with the coding style, to simplify the source code, and to correct the direction in which I'd like to take it in https://github.com/jitsi/jitsi-meet/tree/BeatC-mobile-landing so this PR is no longer necessary (to me). |
This PR is based on #1204 (because moving app initialization is in that PR).
So, this PR provides logic for "mobile landing". It means that if user opens jitsi-meet via mobile device we'll try to detect her platform and if it's a mobile device we'll show corresponding landing page with link to
iOSorAndroidapp correspondingly.Landing for Welcome page:
Landing for Conference:
If user clicks on"Download App" button app redirects to the store (Google Play in my case). Clicking on the second button proceeds previous action (opening welcome or conference page).