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

Docs/finish migration from loopback.io #1141

Merged
merged 1 commit into from
Mar 19, 2018

Conversation

shimks
Copy link
Contributor

@shimks shimks commented Mar 15, 2018

There are a couple of stray pages that haven't been migrated and updated over to the monorepo. The PR fixes this

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Related API Documentation was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in packages/example-* were updated

@shimks shimks force-pushed the docs/finish-migration-from-loopback.io branch 3 times, most recently from bc33a32 to ea9da54 Compare March 15, 2018 20:38
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

This is great; Thanks for updating them @shimks! I was able to verify that you captured the new files that were added when deleting all the content in loopback.io (see loopbackio/loopback.io#643 (comment)) and using the script I've been working on to pull the changes from the monorepo as they are now.

@shimks shimks force-pushed the docs/finish-migration-from-loopback.io branch from ea9da54 to 91ac6b6 Compare March 16, 2018 15:46
Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

@b-admike what changes need to be made to the scripts (if any) that pull in LB4 if we are going to move the lb4_sidebar.yml into the docs folder here?

@b-admike
Copy link
Contributor

@virkt25 it'll just be to move them to their appropriate place, so that references are not broken. Also, @shimks what do you think about moving https://github.com/strongloop/loopback.io/blob/gh-pages/pages/en/lb4/includes/CLI-std-options.md?

@shimks
Copy link
Contributor Author

shimks commented Mar 16, 2018

@b-admike I think I want to keep that where is; it's not a file that needs to be changed frequently, and if we do need to change it, we can deal with that when we need to. Speaking of, we may need to componentize or comment your code in update-lb4-docs.js so that it's easier to modify or add other specific files for anyone that may need to add to the script in the future.

@shimks shimks force-pushed the docs/finish-migration-from-loopback.io branch from 91ac6b6 to 645ea6e Compare March 16, 2018 18:13
@shimks shimks requested a review from jannyHou as a code owner March 16, 2018 18:13
@shimks shimks force-pushed the docs/finish-migration-from-loopback.io branch from 645ea6e to f496919 Compare March 16, 2018 18:46
@b-admike
Copy link
Contributor

Fair enough. I've added comments to the script as you suggested, PTAL :-)

@shimks
Copy link
Contributor Author

shimks commented Mar 16, 2018

@raymondfeng Can I get you to sign off on this before I merge it? Thanks

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

On second thought, I'd like us to keep all site related items inside docs/site (i.e. lb4_sidebar.yml and tables). That also means changing the include blocks in command-line-interface.md to

{% include_relative tables/lb4-artifact-commands.html %}

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I did a quick review and didn't find any obvious problems. As far as I am concerned, this patch LGTM.

Please address comments from other reviewers before landing.

@shimks
Copy link
Contributor Author

shimks commented Mar 19, 2018

@b-admike I agree. l'll make the change

@shimks
Copy link
Contributor Author

shimks commented Mar 19, 2018

Could I get a clarification as to why we need to make the change to include_relative? Is it even possible to parse this html stuff in github? If so, is this accounted for in your PR so that it changes it back to {% include content/lb4-project-commands.html %}?

@b-admike
Copy link
Contributor

b-admike commented Mar 19, 2018

Oh it's because right now they are included from the top level of loopback.io and are inside _includes/content, but I want us to have the tables in pages/en/lb4/includes/tables and include them from there instead. I want to be consistent in where we keep LoopBack 4 related site stuff on loopback.io. What do you think?

@b-admike
Copy link
Contributor

Once we hash out the details in this PR, I'll push up the changes to incorporate what we've done here.

@shimks
Copy link
Contributor Author

shimks commented Mar 19, 2018

That sounds good to me, I'll make the changes.

@shimks shimks force-pushed the docs/finish-migration-from-loopback.io branch from 74dbfaf to 90e5b29 Compare March 19, 2018 15:18
@shimks shimks merged commit 73f9fb1 into master Mar 19, 2018
@shimks shimks deleted the docs/finish-migration-from-loopback.io branch March 19, 2018 17:09
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