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

Feature/25176 bitbucket readme #125

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

kat-kan
Copy link

@kat-kan kat-kan commented Oct 26, 2023

Summary

This PR organizes README.md, admin-guide.md and project assets in a way described here mattermost/mattermost#25176
I have

  • reorganized files (admin guide split to separate file, assets moved to docs/images)
  • updated files that use images (new directory)
  • added outline to both .md files
  • added missing headlines
  • formatted files
  • added badges to README (however, I don't have access to any code coverage report so it's empty for now)

Ticket Link

Fixes mattermost/mattermost#25176
https://mattermost.atlassian.net/browse/MM-55077

* Add badges
* Add new outline
* Move admin guide to new file in docs directory
* Add outline
* Add missing headers
@mattermost-build
Copy link

Hello @kat-kan,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project.

Please help complete the Mattermost contribution license agreement?
Once you have signed the CLA, please comment with /check-cla and confirm that the CLA check is green.

This is a standard procedure for many open source projects.

Please let us know if you have any questions.

We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team.

@kat-kan
Copy link
Author

kat-kan commented Oct 26, 2023

/check-cla

@kat-kan
Copy link
Author

kat-kan commented Oct 26, 2023

image
image

@cwarnermm
Copy link
Contributor

@mickmister - Looking forward to your technical review of this PR -- particularly the image updates.

@kat-kan
Copy link
Author

kat-kan commented Oct 29, 2023

I am also looking forward to the review :)

Copy link
Collaborator

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

The overall direction looks good 👍

There are still a couple of times that need work before we can merge the PR.

docs/images/profile.png Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/admin-guide.md Outdated Show resolved Hide resolved
@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Oct 30, 2023
@cwarnermm
Copy link
Contributor

@kat-kan - You've done a very good job with this PR. Thank you for being so flexible and understanding about the feedback you received during @hanzei's technical review.

@hanzei - Thank you for a most excellent review! :)

Copy link
Contributor

@mickmister mickmister 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 the contribution @kat-kan 🎉

LGTM, given hanzei's requests are fulfilled. Thanks!

@cwarnermm
Copy link
Contributor

@hanzei - I believe the only check remaining is to ensure that the image assets are in the correct location.

@kat-kan
Copy link
Author

kat-kan commented Oct 31, 2023

@cwarnermm I believe they are but builds are failing after moving images because they expect specific location. I am waiting for confirmation that I should revert changes.

@kat-kan
Copy link
Author

kat-kan commented Oct 31, 2023

I reverted the changes related to images, every suggestion was introduced

Copy link
Collaborator

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

We are getting there 🎉

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@kat-kan kat-kan requested a review from hanzei November 3, 2023 12:44
@kat-kan
Copy link
Author

kat-kan commented Nov 5, 2023

@cwarnermm as we are still waiting for @hanzei to accept the changes, can you please consider adding "hacktoberfest-accepted" label? The review period in Hacktoberfest ends tomorrow and PR has to be either merged or have the label to be even considered.

Copy link
Collaborator

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Awesome, thank you @kat-kan 💯

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (84a4030) 15.03% compared to head (29bf6d4) 15.03%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #125   +/-   ##
=======================================
  Coverage   15.03%   15.03%           
=======================================
  Files          13       13           
  Lines        2301     2301           
=======================================
  Hits          346      346           
  Misses       1936     1936           
  Partials       19       19           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hanzei
Copy link
Collaborator

hanzei commented Nov 6, 2023

@mickmister The broken CI is unrelated to the PR. What are the next steps to getting it fixed?

@mickmister
Copy link
Contributor

@hanzei I created #127 to fix the issue

@mickmister
Copy link
Contributor

/update-branch

@mickmister
Copy link
Contributor

@mickmister - Looking forward to your technical review of this PR -- particularly the image updates.

@cwarnermm I'm not sure there are any image updates in the PR?

@mattermost-build
Copy link

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

Thanks @kat-kan! I have just one suggestion to check-in an image that is currently pointing to https://user-images.githubusercontent.com. This was already pointing to that so the change is optional for this PR, though since we are cleaning up the README I think why not.

Your work LGTM 👍 Thanks so much!


### Onboard users
![Bitbucket plugin screenshot](https://user-images.githubusercontent.com/45372453/97643091-114a1500-1a47-11eb-9863-2e0e308706ea.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead have the image committed to this repo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Help Wanted: BitBucket README > Split out admin content to child pages
6 participants