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

Make download links cross-platform #476

Merged
merged 15 commits into from
Sep 24, 2020
Merged

Make download links cross-platform #476

merged 15 commits into from
Sep 24, 2020

Conversation

roryabraham
Copy link
Contributor

Fixes

#287

Tests

  1. Open the app on web, and verify that you see links for Desktop, iOS, and Android in the bottom of the left-hand-nav.
  2. Open the app on desktop, and verify that you see links for Web, iOS, and Android in the bottom of the left-hand-nav.
  3. Open the app on iOS, and verify that you see only one link in the bottom of the left-hand-nav that says "View on web" (added a couple words to fill space when it's the only link).

@roryabraham roryabraham requested a review from a team September 11, 2020 19:24
@roryabraham roryabraham self-assigned this Sep 11, 2020
@botify botify requested review from sketchydroide and removed request for a team and sketchydroide September 11, 2020 19:24
@sketchydroide
Copy link
Contributor

I'll review once you have created the index.js for the desktop and web 😄

@roryabraham
Copy link
Contributor Author

Okay, I've set this up so that we can use .desktop.js/.webify.js file extensions. It was impossible to use .web.js file extensions for our own web-specific code, because that file extension is already used by react-native-web, and Electron relies on react-native-web, so we couldn't exclude that file extension from the webpack build for desktop.

@roryabraham
Copy link
Contributor Author

Had to update the eslintrc because it was complaining about a "missing file extension for import". Basically, without any plain .js files in the module, eslint was unsure about which files could be imported, so we just needed to add the list of javascript file extensions to the configuration.

.eslintrc.js Outdated
node: {
extensions: [
'.js',
'.webify.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "webify" and could we make that just .web.js?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

@roryabraham roryabraham Sep 23, 2020

Choose a reason for hiding this comment

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

Yeah, unfortunately we can't just use web.js, because it is already used by a number of files in react-native-web, and the desktop app needs those files. Therefore, we can't exclude .web.js files outright from the webpack build for the desktop app. So we have to just some other extension to denote web-specific implementation, hence: webify

Copy link
Contributor

Choose a reason for hiding this comment

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

What about .browser.js?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, OK. Yeah, I think browser or website would work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we like .browser.js or website.js?
I actually think I like website better, because Electron essentially is a sandboxed browser, of sorts.

README.md Outdated Show resolved Hide resolved
webpack.common.js Outdated Show resolved Hide resolved
webpack.common.js Outdated Show resolved Hide resolved
webpack.common.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated
node: {
extensions: [
'.js',
'.webify.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

README.md Outdated Show resolved Hide resolved
webpack.common.js Outdated Show resolved Hide resolved
@roryabraham
Copy link
Contributor Author

Okay, I've updated this PR to:

  • Improve the README, and move to a separate section so that we can go into a bit more detail
  • Add some code comments
  • Use index.website.js instead of index.webify.js
  • Use an array of regexes instead of regexUnion (I left the RegexUtils, because it is very general and could still be useful in the future)

src/lib/RegexUtils.js Outdated Show resolved Hide resolved
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants