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

docs: concept focused nav #2302

Merged
merged 9 commits into from
Oct 29, 2024
Merged

docs: concept focused nav #2302

merged 9 commits into from
Oct 29, 2024

Conversation

lukegalbraithrussell
Copy link
Contributor

@lukegalbraithrussell lukegalbraithrussell commented Oct 25, 2024

Summary

Applies to both English and Japanese sites.

The huge file count and lines is because I updated the folder structure to more accurately reflect the new docs organization

  • sorts pages into categories. I followed Kaz's outline with one change: I placed "App Home" in "Interactivity /Shortcuts"
  • consolidates deprecated steps from apps into one page.
  • consolidates getting started pages into one with tabs (HTTP / socket mode)
image

Requirements (place an x in each [ ])

Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.48%. Comparing base (e51960b) to head (2118ff1).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2302   +/-   ##
=======================================
  Coverage   92.48%   92.48%           
=======================================
  Files          36       36           
  Lines        7466     7466           
  Branches      651      651           
=======================================
  Hits         6905     6905           
  Misses        553      553           
  Partials        8        8           

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

@@ -0,0 +1,186 @@
---
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Grabbing this from other PR so I can display Assistant in Nav

Copy link
Member

Choose a reason for hiding this comment

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

If we can merge this change ealier than the assistant page PR, removing this from this PR, merging this PR without assistant page, and then adjusting the assistant page PR to be compatible with this new structure may be smoother

@@ -9,11 +9,7 @@ lang: ja-jp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seratch I tried combining the Socket Mode and HTTP versions. Can you read it to make sure I did so properly?

Comment on lines +117 to +121
.tabs-container {
border: 2px solid var(--ifm-hr-background-color); /* Adjust the color and thickness as needed */
border-radius: 5px; /* To give rounded corners */
padding: 0.5em; /* To add spacing inside the tab */
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This adds an outline around the tabs container so you can tell when the tabbed section ends

label: "Bolt for JavaScript",
className: "sidebar-title",
},
"getting-started",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the sidebar organization

@lukegalbraithrussell lukegalbraithrussell marked this pull request as ready for review October 25, 2024 19:05
@lukegalbraithrussell lukegalbraithrussell requested review from filmaj, seratch and a team October 25, 2024 19:06
@seratch seratch added the discussion M-T: An issue where more input is needed to reach a decision label Oct 28, 2024
@seratch seratch added this to the 4.x milestone Oct 28, 2024
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thank you for working on this so quickly!

@@ -0,0 +1,186 @@
---
Copy link
Member

Choose a reason for hiding this comment

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

If we can merge this change ealier than the assistant page PR, removing this from this PR, merging this PR without assistant page, and then adjusting the assistant page PR to be compatible with this new structure may be smoother

docs/content/getting-started.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Left my first round of comments! Thanks for doing this, I think it's a big improvement.

@@ -4,7 +4,7 @@ lang: en
slug: /concepts/event-listening
---

You can listen to [any Events API event](https://api.slack.com/events) using the `event()` method after subscribing to it in your app configuration. This allows your app to take action when something happens in Slack, like a user reacting to a message or joining a channel.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this isn't exactly part of your PR here but figured I'd mention it: the <detail> at the bottom of this page calls out a specific thing about the message() event listening. Perhaps we should move that to the message-listening.md page instead? I also think we can start 'unfurling' these <detail> blocks, since the pages now are much shorter and more focussed. Often, when interacting with devs about various topics, I find I have to explicitly point them to docs content nested inside these folded-up <detail> sections for content relevant to their issue or question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me!

docs/sidebars.js Outdated
},
{
type: "category",
label: "Interactivity & Shortcuts",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure if this label is descriptive or broad enough; will developers understand the breadth of UI surfaces and features this category contains? Buttons, dropdown menus, modal UIs, shortcuts, slash commands... there's a lot here! What about "App UI & Interactivity"? I think that's slightly broader, and then if we can make sure there are descriptions of each of these UIs/features in each sub-page as an intro blurb, I think that would work well. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think one thing we should call out in this document is that, while these interactivity and shortcut-related app features do require acknowledgement of Slack events, the Events API handlers do not because Bolt automatically acknowledges them. Perhaps this particular convenience/magic needs reinforcement on this page and possibly also on the 'listening to messages' and 'listening to events' pages?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there some end-user or api.slack.com document we can link to for Slash Commands?

@@ -12,7 +12,7 @@ You’ll notice in all `action()` examples, `ack()` is used. It is required to c

:::info

Since v2, message shortcuts (previously message actions) now use the `shortcut()` method instead of the `action()` method. View the [migration guide for V2](/tutorial/migration-v2) to learn about the changes.
Since v2, message shortcuts (previously message actions) now use the `shortcut()` method instead of the `action()` method. View the [migration guide for V2](/migration/migration-v2) to learn about the changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just get rid of this callout; we're on v4 now. No need to link or call out things specific about a version that was last released 4 years ago.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the shortcuts and commands pages, we combine both listening and responding into one doc. Maybe we could do the same for actions? I.e. combine action-listening.md and action-respond.md?

docs/sidebars.js Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to change the title of this document so it's a bit more obvious what it's about. "Options" on its own lacks context. It's about dynamic rendering of dropdown select menu options - I'd like to make that more clear in this document as well as in the sidebar.

docs/sidebars.js Outdated
'advanced/logging',
'advanced/custom-routes',
'advanced/receiver',
"concepts/authorization",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd put the authorization article after the authenticating-with-OAuth one, since the latter is a pre-req for using the former.

@lukegalbraithrussell lukegalbraithrussell added the docs M-T: Documentation work only label Oct 29, 2024
@lukegalbraithrussell lukegalbraithrussell merged commit d3fe549 into main Oct 29, 2024
20 checks passed
@lukegalbraithrussell lukegalbraithrussell deleted the docs-updated-nav branch October 29, 2024 15:23
@filmaj filmaj modified the milestones: 4.x, 4.1 Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion M-T: An issue where more input is needed to reach a decision docs M-T: Documentation work only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants