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: Delete redundant characters. #64

Closed
wants to merge 1 commit into from

Conversation

luc4leone
Copy link

@luc4leone luc4leone commented Apr 23, 2019

Delete in an HTML anchor tag these characters "(", ">)" because they resulted in a typo.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Compliance

  • My change isn't breaking (it doesn't cause existing functionality to change).
  • I have read the CONTRIBUTING document.
  • My code follows the coding style of this project.
  • I have properly formatted my commit messages with cz-cli, or manually, following the Angular Commit Messages Guidelines.
  • I have updated the documentation accordingly, or my changes doesn't require documentation changes.
  • I have added tests to cover my changes, or my changes doesn't require new tests.
  • All new and existing tests pass.

@coveralls
Copy link

coveralls commented Apr 23, 2019

Pull Request Test Coverage Report for Build 263

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.797%

Totals Coverage Status
Change from base Build 252: 0.0%
Covered Lines: 146
Relevant Lines: 148

💛 - Coveralls

Copy link
Collaborator

@sarahdayan sarahdayan left a comment

Choose a reason for hiding this comment

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

Hi @luc4leone and thanks for the contribution! As you've seen in the contributing guidelines, the documentation is generated from the code. Therefore, you shouldn't change the HTML files directly, because they will be overridden by the next build. Instead, you should change the docs in the code.

In that case, I think the issue comes from the fact that the original link has an anchor that has parentheses in them (bad Markdown parsing). However, the following link should work to and not cause issues: https://en.wikipedia.org/wiki/ISO_4217#Treatment_of_minor_currency_units_.28the_.22exponent.22.29

I'll let you fix it in the right place 🙂

@luc4leone
Copy link
Author

Sorry @sarahdayan, I read the contributing guidelines, but clearly not as carefully as I should have. I didn't get the point that the docs are generated from the code. Since I didn't know JSDoc, I read its documentation to understand. After some experimenting I think I identified the right place: it's in the README.md.

I deleted the 2 parenthesis in the url and after executing in my terminal npm run docs, I checked the docs/index.html in the editor and also the rendering in the browser, and everything seems right now.

Copy link
Collaborator

@sarahdayan sarahdayan left a comment

Choose a reason for hiding this comment

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

Thanks @luc4leone, almost there! Your edited docs/index.html is still in the PR. Can you please revert that change before we can merge?

@luc4leone
Copy link
Author

I deleted the first incorrect commit @sarahdayan

@sarahdayan
Copy link
Collaborator

Hi @luc4leone, there are still things to fix:

  1. You need to target the develop branch.
  2. You're currently using the link https://en.wikipedia.org/wiki/ISO_4217#Treatment_of_minor_currency_units_the_%22exponent%22, which doesn't work (if you try it in your browser directly, it won't jump to the right section). You need to use https://en.wikipedia.org/wiki/ISO_4217#Treatment_of_minor_currency_units_.28the_.22exponent.22.29.

Once both are fixed, I'll merge your PR.

@luc4leone
Copy link
Author

Hi @luc4leone, there are still things to fix:

I'll take care of them this week, thanks @sarahdayan!

@sarahdayan
Copy link
Collaborator

@luc4leone Any new on this? 🙂

@luc4leone
Copy link
Author

luc4leone commented Mar 21, 2020

Hey @sarahdayan ;-)

will take care of it tomorrow.

@luc4leone
Copy link
Author

Actually I didn't want to open a new PR @sarahdayan, see #124, but I guess that by targeting develop that's what happened. Was there a way to update this one anyway? (so that next time something similar happen again, I'll be able to do it). Thanks :)

@sarahdayan
Copy link
Collaborator

We can continue on #124 :) Closing this one.

@sarahdayan sarahdayan closed this Mar 25, 2020
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.

None yet

3 participants