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

Fixes on Slack Connector Documentation file #2023

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

Conversation

andrianalazana
Copy link

@andrianalazana andrianalazana commented Apr 15, 2024

Description

This pull request addresses documentation issues in the Slack connector. While navigating through the Slack connector process, I identified several errors in the documentation and meticulously corrected them.

Status

READY

Type of change

  • Documentation (fix or adds documentation)

How Has This Been Tested?

I've connected Opsdroid to Slack twice, paying close attention each time. I found errors and misunderstandings in the documentation, so I suggested improvements.

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Member

@jacobtomlinson jacobtomlinson 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 this @andrianalazana. We recently merged a log of fixes to CI so if you update your PR to the latest commit and resolve the conflicts things should pass here.

docs/connectors/slack.md Outdated Show resolved Hide resolved
@jacobtomlinson
Copy link
Member

Now that #2038 has been merged could you clean this PR up?

@andrianalazana
Copy link
Author

I've made the changes. Let me know if something else is missing! Thanks

@andrianalazana
Copy link
Author

Hi @jacobtomlinson it's been a while. Is theresomething else to fix for thid prq? Pls let me know.

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

I'm afraid things still aren't in a state where this can be merged. The note block needs to be put back to how it was, and the indentation needs fixing.

Also it looks like there are some merge conflicts that need to be resolved before I can merge this in.

Please can you take a look?

Comment on lines 79 to 81
```{note}
While it is not mandatory to configure the Event API before using Slack in Socket Mode, we recommend
setting up the Event API for its full feature set.
Copy link
Member

Choose a reason for hiding this comment

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

This is MyST syntax, but unfortunately we do not use that here. Please can you revert this exactly as it was.

```eval_rst
.. note::
   ...
```

Copy link
Author

Choose a reason for hiding this comment

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

This is how the note was formatted before my changes:

```{note}
You should follow the instructions for the Event API first when configuring your slack connector, even if you are planning
on using slack in Socket Mode.

Shoudn't I put it like this again?

docs/examples/contact-manager.md Outdated Show resolved Hide resolved
@andrianalazana
Copy link
Author

@jacobtomlinson I've made all the necessary changes. However, the Read the Docs build failed. It appears there's a problem with the block declaration using eval_rst which I modified. Additionally, the build failed due to a missing cross-reference target 'usage' in slack.md, which I did not change. Any help resolving these issues would be appreciated.

docs/connectors/slack.md Show resolved Hide resolved
docs/examples/contact-manager.md Outdated Show resolved Hide resolved
@andrianalazana
Copy link
Author

Hi @jacobtomlinson. Have a nice week! Could you review the changes in order to clean this pr?

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.

3 participants