-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
docs(Ads): fix rendering in SSR #3218
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3218 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 169 169
Lines 2790 2790
=======================================
Hits 2788 2788
Misses 2 2 Continue to review full report at Codecov.
|
@@ -10,7 +10,7 @@ const Document = ({ Body, children, Head, Html, siteData: { dev, versions } }) = | |||
<meta name='viewport' content='width=device-width, initial-scale=1, shrink-to-fit=no' /> | |||
|
|||
<link rel='shortcut icon' type='image/x-icon' href='/logo.png' /> | |||
<link rel='stylesheet' href='/style.css' /> | |||
<link rel='stylesheet' href={`/style.css?${versions.suir}`} /> |
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.
We added changes to styles in #3215, but they can be cached because we don't manage these styles by Webpack (i.e. content hash or other hash).
@@ -15,7 +15,8 @@ | |||
"scripts": { | |||
"build": "cross-env NODE_ENV=production gulp build", | |||
"build:changelog": "github_changelog_generator --no-issues --no-unreleased --release-branch master --since-tag $(git describe --abbrev=0 --tags $(git rev-parse HEAD~300))", | |||
"build:docs": "gulp --series build:docs", | |||
"build:docs": "cross-env NODE_ENV=production gulp build:docs", | |||
"build:docs:staging": "cross-env STAGING=true yarn build:docs && yarn serve docs/dist", |
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.
Is this for local development? If so, should it be NODE_ENV=development? If not, why not NODE_ENV=staging?
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.
This for testing production build of docs locally
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.
NODE_ENV=test?
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 main idea that build will be production, so you will have the same result as will be on react.semantic-ui.com
, i.e. manual predeployment check.
The current production build cannot be tested locally because it has hardcoded links to react.semantic-ui.com
.
So we can't use there NODE_ENV
variable, however we can remove this change at all. I thought that it would be useful for you.
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.
deleted
Signed-off-by: Oleksandr Fediashov <ofediashov@exadel.com>
@@ -1,5 +1,5 @@ | |||
require('@babel/register')({ | |||
ignore: [/node_modules/, /docs\/dist/], | |||
ignore: [/node_modules/, /docs[\\/]dist/], |
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.
I also added a fix for build on Windows
@levithomason I merged this PR, feel free to add any changes 👍 |
Released in |
Fixes #3225.
This PR:
document
is not exists in SSR, refs docs(Ads): fix multiple ads #3215scripts
and adds staging mode