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

Protractor migration guide #2812

Closed
wants to merge 27 commits into from
Closed

Conversation

bencodezen
Copy link
Contributor

This PR introduces a new Protractor Migration Guide to the References section to keep in parallel with the current information architecture of where "Migration Guide" lives.

Copy link
Contributor

@kevinold kevinold left a comment

Choose a reason for hiding this comment

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

Looks great!

source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

  1. Overall I'd like to see more inter-linking between our existing docs.

  2. Please make sure to run npm start after updating all the url tags I suggested! I may have a typo in some of my suggestions and the build will catch and warn in the console if it's written to a dead link / incorrect.

  3. I think the headings order is a bit strange. I suggest having "Introduction" as a main heading with the subsections - then Getting Started the next main section.

    To be clear, here's the hierarchy I'm suggesting. No reason to hide this great content that they can jump to.

    Before

    Screen Shot 2020-05-21 at 3 54 31 PM

    After

    Screen Shot 2020-05-21 at 3 54 13 PM

source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
source/guides/references/protractor-migration-guide.md Outdated Show resolved Hide resolved
@bencodezen
Copy link
Contributor Author

@jennifer-shehane Thanks so much for the thorough review! Learned a lot about how we use Hexo for Cypress documentation. Really appreciate it!

@jennifer-shehane
Copy link
Member

@bencodezen There are some errors in the build. We eslint all of the code written in code blocks. The Buffer one is likely too aggressive, so you could add an eslint ignore comment line there.

Screen Shot 2020-05-22 at 1 57 33 PM

@jennifer-shehane jennifer-shehane self-requested a review May 22, 2020 07:30
@bencodezen
Copy link
Contributor Author

Hey @jennifer-shehane! I think everything should be taken care of at this point. I've run a check on both npm start and npm run build locally and they seem to pass without any errors. Let me know if I missed anything else!

bencodezen and others added 3 commits May 28, 2020 12:10
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
bencodezen and others added 2 commits May 28, 2020 12:11
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
@bencodezen
Copy link
Contributor Author

bencodezen commented Jun 5, 2020

@amirrustam @jennifer-shehane I'm a bit stumped on this one. It keeps saying that it can't find the Japanese Sidebar equivalent of the Protractor Migration Guide, but the japanese folder is gitignored and shows up to be properly updated locally. Is there something we need to change in the CI process?

Here's the testing reference: https://dashboard.cypress.io/projects/ma3dkn/runs/12494/specs

Copy link
Contributor

@amirrustam amirrustam left a comment

Choose a reason for hiding this comment

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

@bencodezen so every language has it's own sidebar yml as well. For example, for Japanese, it is the ja.yml file. In it, you will the sidebar config for it.

@bencodezen
Copy link
Contributor Author

@bencodezen so every language has it's own sidebar yml as well. For example, for Japanese, it is the ja.yml file. In it, you will the sidebar config for it.

🤦‍♂️ Didn't realize there was a Japanese sidebar in theme in addition to the source/ja folder which is where I assumed it was coming from. Thanks for the tip!

@jennifer-shehane
Copy link
Member

@bencodezen I'm a little unclear about what is left to do with this PR. Did you have more work you wanted to do? Are you requiring a re-review from one of us?

@bencodezen
Copy link
Contributor Author

@jennifer-shehane Thanks for checking in! It looks like we're going to put this on hold at the moment. I've removed you from the assignee and moving it to @amirrustam so it's no longer on your plate.

@jennifer-shehane jennifer-shehane removed their request for review June 24, 2020 06:23
@jennifer-shehane jennifer-shehane dismissed their stale review June 24, 2020 06:23

Dismissing my previous review as addressed

@amirrustam
Copy link
Contributor

Will revive as part of the pending docs transition.

@amirrustam amirrustam closed this Feb 22, 2021
@matthamil matthamil deleted the protractor-migration-guide branch April 14, 2021 19:56
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