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

Add link to Math object in Freelancer Rates intro #1453

Closed
wants to merge 1 commit into from

Conversation

StreakInTheSky
Copy link

Since the exercise used for Numbers and Arithmetic Operators is combined, if the user were to come into it from Arithmetic Operations without reading the Numbers concept, they would not be introduced to the Math object, which is necessary to complete the exercise. It's mentioned in the hints, but I don't think it's enough for someone new to JS.

I tried to use the phrasing and structure of the related docs, but let me know if there's a better way to do it.

Adds a link to the Math built-in object on MDN
@github-actions
Copy link
Contributor

Dear StreakInTheSky

Thank you for contributing to the JavaScript track on Exercism! 💙
You will see some automated feedback below 🤖. It would be great if you can make sure your PR covers those points. This will save your reviewer some time and your change can be merged quicker.

  • 📜 The following files usually contain very similar content.

    • concepts/<concept>/about.md
    • concepts/<concept>/introduction.md
    • exercises/concept/<exercise>/.docs/introduction.md

    Please check whether the changes you made to one of these also need to be applied to the others.

  • ✍️ If your PR is not related to an existing issue (and is not self-explaining like a typo fix), please make sure the description explains why the change you made is necessary.

  • 🔤 If your PR fixes an easy to identify typo, if would be great if you could check for that typo in the whole repo. For example, if you found Unicdoe, use "replace all" in your editor (or command line magic) to fix it consistently.

Dear Reviewer/Maintainer

  • 📏 Make sure you set the appropriate x:size label for the PR. (This also works after merging, in case you forgot about it.)

  • 🔍 Don't be too nit-picky. If the PR is a clear improvement compared to the status quo, it should be approved as clear signal this is good to be merged even if the minor comments you might have are not addressed by the contributor. Further improvement ideas can be captured in issues (if important enough) and implemented via additional PRs.

  • 🤔 After reviewing the diff in the "Files changed" section, take a moment to think about whether there are changes missing from the diff. Does something need to be adjusted in other places so the code or content stays consistent?

Automated comment created by PR Commenter 🤖.

@junedev
Copy link
Member

junedev commented Oct 19, 2021

@StreakInTheSky Someone is currently working on cleaning up freelancer rates, see #1049. Part of that is also to point to the built-in math object for the rounding. (Its one of the items in the todo list there). I don't want to make things difficult for the person working on this so I will close this PR and let them do their work instead. Hope this is ok for you.

As for the math package in general, there will be a separate exercise on this in the future.

Regarding you concern about "coming from arithmetic operators": When you actually start the exercise, you see an introduction document that includes the content of all the introductions of the concepts that are taught by this exercise. So it doesn't matter from which concept you started.

@junedev junedev closed this Oct 19, 2021
@StreakInTheSky
Copy link
Author

Oh ok, sounds good.

About your last point, the Math object isn’t actually linked in the exercise which is what I was adding. But looks like that will be fixed.

@StreakInTheSky StreakInTheSky deleted the patch-1 branch October 19, 2021 15:45
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.

2 participants