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

Move showcase source data into a JSON file #12626

Closed
wants to merge 4 commits into from
Closed

Move showcase source data into a JSON file #12626

wants to merge 4 commits into from

Conversation

hramos
Copy link
Contributor

@hramos hramos commented Feb 28, 2017

The showcase is displayed in the homepage as well as in a standalone showcase page. The source data for these two pages is embedded in each page, resulting in repetition of data across both.

This PR splits out all of the showcase data to its own file, which is then loaded into the Metadata component during website generation.

Test Plan

cd website && npm start, then verified index and showcase loaded successfully
cd website && node server/generate.js, confirmed script runs successfully

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Feb 28, 2017
Copy link
Contributor

@ericvicenti ericvicenti left a comment

Choose a reason for hiding this comment

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

Looking great!

"linkPlayStore": "https://play.google.com/store/apps/details?id=com.cbssports.fantasy.franchisefootball2015",
"infoLink": "http://www.cbssports.com/fantasy/football/games/franchise/2015",
"infoTitle": "Award winning Fantasy Football league manager",
"pinned": false
Copy link
Contributor

Choose a reason for hiding this comment

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

having pinned=false seems verbose. You could just leave it off unless it is actually pinned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my original approach, but I could not find a clean way to filter for unpinned entries in https://github.com/facebook/react-native/pull/12626/files#diff-deb30c36b81d11c5a077a65d4e2d9baaR60 in that case.

Variations I tried that failed to only return objects missing the pinned key (or have pinned: false):

const featuredApps = showcaseApps.filter(app => {
  return !app.hasOwnProperty('pinned') || !app.pinned;
}).sort(function(a, b) {
  return a.name.localeCompare(b.name);
});

@@ -179,6 +180,13 @@ function execute(options) {
metadatas.config[key] = process.env[key];
});

// load showcase apps into metadata
var showcaseApps = JSON.parse(fs.readFileSync(
Copy link
Contributor

Choose a reason for hiding this comment

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

in other codebases we would want flow types, but nevermind here 😄

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Mar 1, 2017
@facebook-github-bot
Copy link
Contributor

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

2 similar comments
@facebook-github-bot
Copy link
Contributor

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@hramos hramos deleted the showcase-tests branch March 6, 2017 17:45
hramos added a commit that referenced this pull request Mar 9, 2017
Summary:
The showcase is displayed in the homepage as well as in a standalone showcase page. The source data for these two pages is embedded in each page, resulting in repetition of data across both.

This PR splits out all of the showcase data to its own file, which is then loaded into the Metadata component during website generation.

`cd website && npm start`, then verified index and showcase loaded successfully
`cd website && node server/generate.js`, confirmed script runs successfully
Closes #12626

Differential Revision: D4632036

Pulled By: hramos

fbshipit-source-id: 2d1ad890e78e457205179e36c3ef04ffec354ad9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants