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.

Sign up for GitHub

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

Jump to bottom

Tweniee/Isssue:#11361 Updated Document for PR release #11512

Merged
merged 9 commits into from
Nov 28, 2023
Merged

Tweniee/Isssue:#11361 Updated Document for PR release #11512

merged 9 commits into from
Nov 28, 2023

Conversation

Tweniee
Copy link
Contributor

@Tweniee Tweniee commented Nov 9, 2023 •
edited
Loading

Summary

Incorporated rebase functionality into the documentation
Previous PR:#11509

References

Issue:#11361

Reviewer guidance

Its a documentation change onl

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@MisRob MisRob self-requested a review November 9, 2023 08:20
@MisRob MisRob added the TODO: needs review Waiting for review label Nov 9, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 9, 2023 •
edited
Loading

Build Artifacts

Asset type Download link
PEX file kolibri-0.16.0b6.dev0_git.532.gee4af3f2.pex
Windows Installer (EXE) kolibri-0.16.0b6.dev0+git.532.gee4af3f2-unsigned.exe
Mac Installer (DMG) kolibri-0.16.0b6.dev0+git.532.gee4af3f2-0.3.0.dmg
Android Package (APK) kolibri-0.16.0b6.dev0+git.532.gee4af3f2-0.1.0-debug.apk
TAR file kolibri-0.16.0b6.dev0+git.532.gee4af3f2.tar.gz
WHL file kolibri-0.16.0b6.dev0+git.532.gee4af3f2-py2.py3-none-any.whl

MisRob
MisRob requested changes Nov 14, 2023
View reviewed changes
Copy link
Member

@MisRob MisRob left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @Tweniee, thank you for your contribution! I've left a few notes.

Also, I don't see the new page on the documentation preview https://kolibri-dev--11512.org.readthedocs.build/en/11512/ (this link is available in the checks section of this pull request). It needs to show in the menu as well as in the list of the "How To Guides" page:

docs

Were you able to run the documentation locally and preview it?

docs/howtos/Release_Process_with_PR_Rebase.md Outdated Show resolved Hide resolved
docs/howtos/Release_Process_with_PR_Rebase.md Outdated Show resolved Hide resolved
docs/howtos/Release_Process_with_PR_Rebase.md Outdated Show resolved Hide resolved
docs/howtos/Release_Process_with_PR_Rebase.md Outdated Show resolved Hide resolved
@Tweniee Tweniee requested a review from MisRob November 15, 2023 04:33
@Tweniee
Copy link
Contributor Author

Tweniee commented Nov 15, 2023

@MisRob made the required changes and improved few statance according to your changes please do review it and thanks for your time and suggestions

@MisRob
Copy link
Member

MisRob commented Nov 17, 2023

Thanks @Tweniee, I will re-review

MisRob
MisRob requested changes Nov 20, 2023
View reviewed changes
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

Thanks for your cooperation @Tweniee, it's much better. Not completely there yet though, I left a few notes. Let me know if you had any questions.

docs/howtos/rebasing_a_pull_request.md Outdated Show resolved Hide resolved
docs/howtos/rebasing_a_pull_request.md Outdated

Locally, checkout your feature branch (that you previously created off the develop):
```
git checkout develop
Copy link
Member

Choose a reason for hiding this comment

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

A feature branch is not the develop branch, so please remove this line, it's not a step that people should do. I've tried to explain this a bit in the previous review, you're welcome to ask any clarifying questions if it's not clear.

docs/howtos/rebasing_a_pull_request.md Outdated Show resolved Hide resolved
docs/howtos/rebasing_a_pull_request.rst Outdated Show resolved Hide resolved
docs/howtos/rebasing_a_pull_request.rst Outdated Show resolved Hide resolved
docs/howtos/rebasing_a_pull_request.md Outdated
@@ -0,0 +1,32 @@
# Rebasing a Pull Request
Copy link
Member

Choose a reason for hiding this comment

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

I believe the primary title will be shown on the generated page as the result of being automatically propagated from the corresponding .rst file so this would cause it to appear two times. Therefore, let's remove it.

@Tweniee Tweniee requested a review from MisRob November 20, 2023 16:44
@Tweniee
Copy link
Contributor Author

Tweniee commented Nov 20, 2023

I guess this will do the work @MisRob
thanks for your time and sorry for re reviewing again 👍

@MisRob
Copy link
Member

MisRob commented Nov 22, 2023

Thanks for following-up, @Tweniee. I can see two remaining feedback items that still need to be resolved.

@Tweniee
Copy link
Contributor Author

Tweniee commented Nov 22, 2023

@MisRob Sorry for that, i missed it. now i guess it look fine.

@MisRob
Copy link
Member

MisRob commented Nov 24, 2023

Thanks @Tweniee. I will re-review some time next week.

@Tweniee
Copy link
Contributor Author

Tweniee commented Nov 24, 2023

Sure @MisRob 😃

MisRob
MisRob reviewed Nov 28, 2023
View reviewed changes
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

It's looking good @Tweniee! Thank you for all work. One last thing before we can merge - the linting check is failing. Could you run pre-commit as described in this section of the developer documentation?

@Tweniee
Copy link
Contributor Author

Tweniee commented Nov 28, 2023

@MisRob linting is done just one Add Contributor to AUTHORS.md / add_contributor (pull_request) is pending i am not sure, should i have to do it or not?

@Tweniee Tweniee requested a review from MisRob November 28, 2023 14:58
@MisRob
Copy link
Member

MisRob commented Nov 28, 2023

@Tweniee Thank you! No the other check is okay, it's a problem on our side. I will deal with it.

@MisRob MisRob merged commit 7c6b566 into learningequality:develop Nov 28, 2023
33 of 35 checks passed
@MisRob MisRob mentioned this pull request Dec 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MisRob MisRob MisRob approved these changes

Assignees
No one assigned
Labels
SIZE: small TODO: needs review Waiting for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Tweniee @MisRob