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

Added viewport support and defered execution on gopherjs serve #860

Merged
merged 2 commits into from Oct 3, 2018

Conversation

Projects
None yet
3 participants
@AnikHasibul
Contributor

AnikHasibul commented Sep 30, 2018

viewport

Added <meta name="viewport" content="initial-scale=1"> for responsive web development environment with gopherjs via gopherjs serve command!

Deferred execution

<script defer src="`+base+`.js"></script>`

Added defer to execute the script as soon as the page load finished!

Show outdated Hide outdated tool.go Outdated
@hajimehoshi

I don't have a strong opinion on this change, then lgtm.

@hajimehoshi

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Oct 3, 2018

Member

I think it is ok to merge this, and revert this if we find problems.

Member

hajimehoshi commented Oct 3, 2018

I think it is ok to merge this, and revert this if we find problems.

@hajimehoshi hajimehoshi merged commit bf5fc7d into gopherjs:master Oct 3, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 3, 2018

Member

I tried to add defer to the script in #570, but aborted because @neelance pointed out a valid problem with it in #570 (comment). I confirmed it in #570 (comment).

Then I opened #571 to discuss the situation and look for a good solution that didn't compromise on the no-flash goal.

It seems this PR is a duplicate of #570, except it also adds a viewport and whitespace.

Sorry I didn't see it earlier @hajimehoshi.

Member

dmitshur commented Oct 3, 2018

I tried to add defer to the script in #570, but aborted because @neelance pointed out a valid problem with it in #570 (comment). I confirmed it in #570 (comment).

Then I opened #571 to discuss the situation and look for a good solution that didn't compromise on the no-flash goal.

It seems this PR is a duplicate of #570, except it also adds a viewport and whitespace.

Sorry I didn't see it earlier @hajimehoshi.

@hajimehoshi

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Oct 3, 2018

Member

Ah I understood. Sorry I didn't wait for your reviews. Should we revert this?

Member

hajimehoshi commented Oct 3, 2018

Ah I understood. Sorry I didn't wait for your reviews. Should we revert this?

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 4, 2018

Member

Should we revert this?

Yes, I think so. The problems with this approach are already known, so it's not necessary to wait to discover them. It's hard to find a good default index.html that will work for everyone needs, so I think it's fair to supply a minimal one. Projects can always provide their own custom index.html if they need to override some behavior.

Sorry I didn't wait for your reviews.

No problem, I'm glad you're taking action and following up on it!

Member

dmitshur commented Oct 4, 2018

Should we revert this?

Yes, I think so. The problems with this approach are already known, so it's not necessary to wait to discover them. It's hard to find a good default index.html that will work for everyone needs, so I think it's fair to supply a minimal one. Projects can always provide their own custom index.html if they need to override some behavior.

Sorry I didn't wait for your reviews.

No problem, I'm glad you're taking action and following up on it!

hajimehoshi added a commit that referenced this pull request Oct 4, 2018

hajimehoshi added a commit that referenced this pull request Oct 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment