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

Convert cli-guides to be a single repo project #28

Merged
merged 9 commits into from
Nov 10, 2018
Merged

Conversation

mansona
Copy link
Member

@mansona mansona commented Nov 4, 2018

Howdy folks 👋

When we set up the cli-guides-source it was using a very early version of Guidemaker that required the source and the app to be in different repos. This is because of the fact that Guidemaker is essentially an extraction of the tech behind the Ember Guides App and that was one of the original design principles of that project.

Having different repos was useful in some regards but caused a number of issues:

  • we are not able to preview pull requests in the "source only" repo. This is because the preview can only happen after a new version of the source has been released
  • deploying a new version of the guides to production requires deploying a markdown-only npm package and updating that dependency on the consuming app
  • seeing your changes locally requires a linking process that may be confusing to new contributors

This PR solves all of these problems, and as a bonus you will be able to see a netlify demo of this PR to show you what it looks like 🎉

I am happy to discuss this some more if anyone has any questions 👍

@mansona mansona force-pushed the feature/single-repo branch 2 times, most recently from f528531 to d9ea8bd Compare November 4, 2018 23:14
@mansona
Copy link
Member Author

mansona commented Nov 7, 2018

For anyone who wants to have a look at the deploy-preview it's here: https://deploy-preview-28--ember-cli-guide.netlify.com

I think the name of the app was changed since the build notification was added ☝️

app/index.html Outdated
<head>
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<title>CliGuidesSource</title>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<title>CliGuidesSource</title>
<title>Ember CLI Guides</title>

@Gaurav0
Copy link
Contributor

Gaurav0 commented Nov 7, 2018

Also, let's update the README.md with the new simple instructions to running the app.

@mansona
Copy link
Member Author

mansona commented Nov 7, 2018

@Gaurav0 I have updated the README with the simpler instructions 👍

@locks nice catch 🎉 the right move here was to remove the title from that document because it's handled by guidemaker internals (because it needs to add the current page to the title dynamically) 👍

Copy link
Contributor

@Gaurav0 Gaurav0 left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. @locks ?

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jenweber jenweber left a comment

Choose a reason for hiding this comment

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

Ran locally, looks good 👍 ! Excited to be able to have review apps.

@jenweber jenweber merged commit bab176a into master Nov 10, 2018
@delete-merged-branch delete-merged-branch bot deleted the feature/single-repo branch November 10, 2018 15:54
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.

4 participants