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

Bugfix/clean up #24

Merged
merged 4 commits into from Nov 26, 2019

Conversation

anton-karlovskiy
Copy link
Contributor

README is updated (minor).

README.md Outdated
@@ -188,31 +187,31 @@ const Component = React.lazy(() => {

let module;
switch (effectiveType) {
case "3g":
module = import(/* webpackChunkName: "light" */ "./Light.js");
case 'slow-2g':
Copy link
Collaborator

Choose a reason for hiding this comment

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

I intentionally excluded the other supported values here as they were making the code-snippet look a little on the long side (I imagine most users would use 2 values) 😄

That said, perhaps a good middle ground is a line after each code-snippet that notes the supported return values? e.g 2g, 3g etc. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got you.

So for Network, Save Data, CPU Cores / Hardware Concurrency, Memory, after code snippet, we can add some line that notes the supported return values.

For example for Network, effectiveConnectionType values can be slow-2g, 2g, 3g, and 4g.

Am I correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  switch (effectiveType) {
    case '3g':
      module = import(/* webpackChunkName: "light" */ './Light.js');
      break;
    case '4g':
      module = import(/* webpackChunkName: "full" */ './Full.js');
      break;
    default:
      module = import(/* webpackChunkName: "full" */ './Full.js');
      break;
  }

I thought that if effective connection type is 2g, the user might end up downloading Full.js.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For example for Network, effectiveConnectionType values can be slow-2g, 2g, 3g, and 4g.

Yes, exactly!

Copy link
Collaborator

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments.

@addyosmani addyosmani self-assigned this Nov 20, 2019
@addyosmani
Copy link
Collaborator

@anton-karlovskiy This should be good to land soon. Would you like to go ahead with making those minor changes discussed in the comments?

@anton-karlovskiy
Copy link
Contributor Author

@anton-karlovskiy This should be good to land soon. Would you like to go ahead with making those minor changes discussed in the comments?

Yes, it's done. Could you please review it?

Copy link
Collaborator

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

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

LGTM

@addyosmani addyosmani merged commit 9382fa9 into GoogleChromeLabs:master Nov 26, 2019
@anton-karlovskiy anton-karlovskiy deleted the bugfix/clean-up branch November 26, 2019 20:52
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.

None yet

2 participants