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

Update Getting Started DocC Documentation for the Firebase Emulator #313

Merged
merged 15 commits into from
Oct 2, 2024

Conversation

Vai-Man
Copy link
Contributor

@Vai-Man Vai-Man commented Oct 1, 2024

What it Does

How I Tested

  • N/A

Notes

  • N/A

Screenshot

  • N/A

setup the firebase emulator
made some changes about flutter emulator setup
added firebase emulator setup tutorial
@Vai-Man Vai-Man requested a review from mikaelacaron as a code owner October 1, 2024 17:26
@Vai-Man Vai-Man closed this Oct 1, 2024
@Vai-Man Vai-Man reopened this Oct 1, 2024
@mikaelacaron mikaelacaron changed the title Fixed issue #311 Update Getting Started DocC Documentation for the Firebase Emulator Oct 1, 2024
Copy link
Owner

@mikaelacaron mikaelacaron left a comment

Choose a reason for hiding this comment

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

You're almost there! I have several comments that need resolved mainly related to formatting. If you have any questions, you can comment directly on that thread, otherwise click the resolve button once you've addressed my comment. Once you fix everything, please click the re-review button

And note that the "Start Working on an Issue" section should be updated as well, so that it matches the contributing file. showing a screenshot of the Firebase emulator would be great too!

You don't need to close the PR, just make your changes, commit and push them, the PR will update automatically

Re-review button

@Vai-Man Vai-Man requested a review from mikaelacaron October 1, 2024 19:18
Copy link
Owner

@mikaelacaron mikaelacaron left a comment

Choose a reason for hiding this comment

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

Please resolve these issues and let me know if you have any questions! when you're done again, click the re-review button, thanks!

Re-review button

README.md Outdated
@@ -3,7 +3,7 @@
![GitHub last commit (branch)](https://img.shields.io/github/last-commit/mikaelacaron/Basic-Car-Maintenance/dev?logo=github)
![GitHub contributors](https://img.shields.io/github/contributors/mikaelacaron/Basic-Car-Maintenance)
[![first-timers-only](https://img.shields.io/badge/first--timers--only-friendly-blue.svg)](https://www.firsttimersonly.com/)
[![Unit Tests](https://github.com/mikaelacaron/Basic-Car-Maintenance/actions/workflows/unit-tests.yml/badge.svg?event=push)](https://github.com/mikaelacaron/Basic-Car-Maintenance/actions/workflows/unit-tests.yml)
[![Unit Tests]()](https://github.com/mikaelacaron/Basic-Car-Maintenance/actions/workflows/unit-tests.yml)
Copy link
Owner

@mikaelacaron mikaelacaron Oct 1, 2024

Choose a reason for hiding this comment

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

What's this do? it's not described in the PR description what this change is for, and it's unrelated to this PR, if you want to change something unrelated, make a new issue describing what the change is and why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That happened unintentionally due to oversight! Apologies.

Copy link
Owner

Choose a reason for hiding this comment

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

This still shows as changed

Copy link
Owner

Choose a reason for hiding this comment

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

nothing in the README.md file should be changed

@@ -88,6 +88,53 @@
}
}
}

@Section(title: "Setting Up Firebase Local Emulator") {
Copy link
Owner

Choose a reason for hiding this comment

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

You need to build the documentation locally, step 1 doesn't have a line braek between the text and then the command
Screenshot 2024-10-01 at 8 31 56 PM

step 2 and many others are now missing text because using --- does not work for an interactive tutorial
Screenshot 2024-10-01 at 8 31 59 PM

Here's Apple's documentation about how to build a DocC interactive tutorial
https://www.swift.org/documentation/docc/building-an-interactive-tutorial

Copy link
Owner

Choose a reason for hiding this comment

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

while this now renders better, many new warnings are created because using underscores doesn't fix this issue. Refer to the documentation to properly format the document

Screenshot 2024-10-01 at 9 31 10 PM

@Vai-Man Vai-Man requested a review from mikaelacaron October 1, 2024 20:00
Copy link
Owner

@mikaelacaron mikaelacaron left a comment

Choose a reason for hiding this comment

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

It doesn't seem like you ran this locally to fix the issues, see attached screenshot with the 10+ warnings happened because of a result of your change

this is the last time I'm going to review this, or this PR will be marked as spam.

The README was also not resolved and is still showing changes, when it shouldn't

@@ -88,6 +88,53 @@
}
}
}

@Section(title: "Setting Up Firebase Local Emulator") {
Copy link
Owner

Choose a reason for hiding this comment

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

while this now renders better, many new warnings are created because using underscores doesn't fix this issue. Refer to the documentation to properly format the document

Screenshot 2024-10-01 at 9 31 10 PM

@Vai-Man
Copy link
Contributor Author

Vai-Man commented Oct 2, 2024

It doesn't seem like you ran this locally to fix the issues, see attached screenshot with the 10+ warnings happened because of a result of your change

I apologize for the oversight. I was away from my PC and had to make the changes manually while referring to the documentation and other guides.

this is the last time I'm going to review this, or this PR will be marked as spam.

I apologise for this, I am still learning and will ensure I follow the guidelines more carefully.

The README was also not resolved and is still showing changes, when it shouldn't

I believe the issue has been resolved now. Please let me know if it’s not, and I would be happy to make any necessary adjustments.

@Vai-Man Vai-Man requested a review from mikaelacaron October 2, 2024 09:08
README.md Outdated
@@ -3,7 +3,7 @@
![GitHub last commit (branch)](https://img.shields.io/github/last-commit/mikaelacaron/Basic-Car-Maintenance/dev?logo=github)
![GitHub contributors](https://img.shields.io/github/contributors/mikaelacaron/Basic-Car-Maintenance)
[![first-timers-only](https://img.shields.io/badge/first--timers--only-friendly-blue.svg)](https://www.firsttimersonly.com/)
[![Unit Tests](https://github.com/mikaelacaron/Basic-Car-Maintenance/actions/workflows/unit-tests.yml/badge.svg?event=push)](https://github.com/mikaelacaron/Basic-Car-Maintenance/actions/workflows/unit-tests.yml)
[![Unit Tests]()](https://github.com/mikaelacaron/Basic-Car-Maintenance/actions/workflows/unit-tests.yml)
Copy link
Owner

Choose a reason for hiding this comment

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

nothing in the README.md file should be changed

@mikaelacaron
Copy link
Owner

this is the last time I'm going to review this, or this PR will be marked as spam.

I apologise for this, I am still learning and will ensure I follow the guidelines more carefully.

I've made the changes for merging this PR. Thank you for contributing. I am fully open to having beginners contribute to this repo. After seeing two revisions of your PR with minimal changes, and not addressing my comments, this seemed like a spam PR. (a problem with Hacktoberfest has overall)

I do hope you continue to contribute and that this isn't discouraging. Is there something in the wording of my comments when reviewing your PR, that I can do to make it more clear what I was looking for you to change?

@mikaelacaron mikaelacaron merged commit 2a5dc70 into mikaelacaron:dev Oct 2, 2024
1 of 2 checks passed
@Vai-Man Vai-Man deleted the firebase-emulator-patch-1 branch October 2, 2024 18:10
@Vai-Man
Copy link
Contributor Author

Vai-Man commented Oct 2, 2024

this is the last time I'm going to review this, or this PR will be marked as spam.

I apologise for this, I am still learning and will ensure I follow the guidelines more carefully.

I've made the changes for merging this PR. Thank you for contributing. I am fully open to having beginners contribute to this repo. After seeing two revisions of your PR with minimal changes, and not addressing my comments, this seemed like a spam PR. (a problem with Hacktoberfest has overall)

Thank you for merging the PR and for your understanding. I’m still learning and trying to get the process right, and I realize now that I should have been more thorough in addressing each point.

I do hope you continue to contribute and that this isn't discouraging. Is there something in the wording of my comments when reviewing your PR, that I can do to make it more clear what I was looking for you to change?

Your comments were clear, but I might have misunderstood some of the details, which led to the missed corrections. Though, I wasn't able to figure a way out to add a horizontal line break in the linked documentation you provided.

@mikaelacaron
Copy link
Owner

You should've looked how it's done in the previous steps that I referenced. Building the project allows you to see how it was done in steps that were already existing

Screenshot 2024-10-01 at 7 13 23 PM

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.

Update Getting Started DocC Documentation
2 participants