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

Enable pass attributes to script and link #937

Merged
merged 1 commit into from
Sep 3, 2018

Conversation

wszgxa
Copy link
Contributor

@wszgxa wszgxa commented Sep 1, 2018

Motivation

Github issue: #848

Enable pass attributes to script tag and link tag.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

I have tested this locally, it will apply the attributes to script and link.

Related PRs

nope

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 1, 2018
@wszgxa wszgxa force-pushed the add-attribut-for-style-script branch from 586b972 to 47eff81 Compare September 1, 2018 13:18
@wszgxa
Copy link
Contributor Author

wszgxa commented Sep 1, 2018

Anyone help to click the rerun button in CI? 😆

@yangshun
Copy link
Contributor

yangshun commented Sep 1, 2018

Retried Netlify deploy!

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Sep 1, 2018

Deploy preview for docusaurus-preview ready!

Built with commit fdb0297

https://deploy-preview-937--docusaurus-preview.netlify.com

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

Just some nits

lib/core/Head.js Outdated
))}
this.props.config.scripts.map(source => {
if (source instanceof Object) {
return source.src ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Inline If with Logical && Operator

e.g:

return source.src && <script type="text/javascript" key={source.src} {...source} />

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @wszgxa! The approach is sound. Made some comments ;)

@@ -155,15 +155,15 @@ h1 {

* `separate` - The secondary navigation is a separate pane defaulting on the right side of a document. See http://docusaurus.io/docs/en/translation.html for an example.

`scripts` - Array of JavaScript sources to load. The script tag will be inserted in the HTML head.
`scripts` - Array of JavaScript sources to load. You can pass only src link as srting, or you can pass attributes as object. The script tag will be inserted in the HTML head.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in srting. Use this instead:

Array of JavaScript sources to load. The values can be either strings or plain objects of attribute-value maps. Refer to the example below. The script tag will be inserted in the HTML head.


`separateCss` - Directories inside which any `css` files will not be processed and concatenated to Docusaurus' styles. This is to support static `html` pages that may be separate from Docusaurus with completely separate styles.

`scrollToTop` - Set this to `true` if you want to enable the scroll to top button at the bottom of your site.

`scrollToTopOptions` - Optional options configuration for the scroll to top button. You do not need to use this, even if you set `scrollToTop` to `true`; it just provides you more configuration control of the button. You can find more options [here](https://github.com/vfeskov/vanilla-back-to-top/blob/v7.1.14/OPTIONS.md). By default, we set the zIndex option to 100.

`stylesheets` - Array of CSS sources to load. The link tag will be inserted in the HTML head.
`stylesheets` - Array of CSS sources to load. You can pass only href link as srting, or you can pass attributes as object. The link tag will be inserted in the HTML head.
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar changes.

lib/core/Head.js Outdated
this.props.config.scripts.map(source => (
<script type="text/javascript" key={source} src={source} />
))}
this.props.config.scripts.map(source => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better written as:

this.props.config.scripts.map(script => {
  const scriptProps = Object.assign({}, 
    { type: 'text/javasript' }, 
    script instanceOf 'Object' ? script : { src: source },
  );
  return <script {...source} />;
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to include the key,

this.props.config.scripts && 
  this.props.config.scripts.map(source => {
    const scriptProps = Object.assign(
        {type: 'text/javasript'},
        source.src ? source : {src: source} // include key in this line is more complexity than use ?..: ...
    );
    return <script {...scriptProps} />;
})

I guess if we don't need key, we can do as your example.

lib/core/Head.js Outdated
this.props.config.stylesheets.map(source => (
<link rel="stylesheet" key={source} href={source} />
))}
this.props.config.stylesheets.map(source => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rewrote here in a similar fashion as well.

@wszgxa
Copy link
Contributor Author

wszgxa commented Sep 3, 2018

Will address comments at tonight.

@wszgxa wszgxa force-pushed the add-attribut-for-style-script branch 2 times, most recently from 74bdea4 to e7b3d14 Compare September 3, 2018 12:46
@wszgxa wszgxa force-pushed the add-attribut-for-style-script branch from e7b3d14 to fdb0297 Compare September 3, 2018 12:52
@wszgxa
Copy link
Contributor Author

wszgxa commented Sep 3, 2018

Hi, guys

I have addressed comments. Because we need to pass down a unique key.

I guess use ternary operator is more readable than use Object.assign.

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Looks fine. Thanks @wszgxa!

@yangshun yangshun merged commit 969399c into facebook:master Sep 3, 2018
@wszgxa wszgxa deleted the add-attribut-for-style-script branch September 4, 2018 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants