Skip to content

Conversation

@mwbrooks
Copy link
Member

@mwbrooks mwbrooks commented Apr 4, 2025

Summary

This pull request adds the Slack CLI logo to the title of the README.md file to match slackapi/bolt-js and slackapi/bolt-python.

Reviewers

  • Preview of README.md
  • Updated .gitignore to no longer ignore the docs/*
    • We were ignoring the docs/* because the command docgen uses docs/ as the default directory
    • If we start to store our public docs in this repo, we'll need to use the docs/ directory as well
    • So, I felt it's okay to stop ignoring this directory
    • Thoughts?
  • Logo is offset in Chrome on macOS
    • Do you notice that the logo is offset vertically a little bit?
    • If I add margin-bottom: -0.1em; then it fixes it on my system, but this could be different on each system
    • I'm leaning toward just keeping it as being a little offset
    • Thoughts?

Requirements

@mwbrooks mwbrooks added docs M-T: Documentation work only semver:patch Use on pull requests to describe the release version increment labels Apr 4, 2025
@mwbrooks mwbrooks added this to the Next Release milestone Apr 4, 2025
@mwbrooks mwbrooks self-assigned this Apr 4, 2025
@mwbrooks mwbrooks marked this pull request as ready for review April 4, 2025 05:04
@mwbrooks mwbrooks requested review from a team as code owners April 4, 2025 05:04
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has checked 503 files.

Valid Invalid Ignored Fixed
460 1 42 0
Click to see the invalid file list
  • docs/static/img/slack-cli-logo.svg
Use this command to fix any missing license headers
```bash

docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix

</details>

@codecov
Copy link

codecov bot commented Apr 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.80%. Comparing base (fad2c14) to head (15bd800).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main       #9   +/-   ##
=======================================
  Coverage   62.80%   62.80%           
=======================================
  Files         210      210           
  Lines       22053    22053           
=======================================
+ Hits        13850    13851    +1     
+ Misses       7126     7125    -1     
  Partials     1077     1077           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

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

Sweet 💯

Attention to detail makes this project feel Slacky

@@ -1,4 +1,4 @@
# Slack CLI
# Slack CLI <img src="docs/static/img/slack-cli-logo.svg" alt="Slack CLI logo" style="height: 1em; max-width: 100%" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Just personnel preference but also sharing some of my past explorations, the title can be centered by using html tags as well, bolt-python, python-sdk, python-hooks)

Suggested change
# Slack CLI <img src="docs/static/img/slack-cli-logo.svg" alt="Slack CLI logo" style="height: 1em; max-width: 100%" />
<h1 align="center">Slack CLI <img src="docs/static/img/slack-cli-logo.svg" alt="Slack CLI logo" style="height: 1em; max-width: 100%" /></h1>

Copy link
Member Author

Choose a reason for hiding this comment

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

The center alignment is a nice touch of the Python projects! 👌🏻

I've tried it with this README.md and it doesn't look quite as slick. Below, I've centered the title, status badges, and quote description.

What are your thoughts on this style @WilliamBergamin @lukegalbraithrussell? My gut says all 3 should be centered otherwise none. The quote description looks funny, but it looks better if I switch it to italics.

Centered Title:

image

Centered Title & Status:

image

Centered Title, Status, and Description:

image

Centered Title, Status, and Description (Italics):

image

Copy link
Member

Choose a reason for hiding this comment

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

📝 Adding onto this to prefer using # for title headings and following sections for standard parsings that some readers might use:

https://github.com/artempyanykh/marksman/blob/main/docs/features.md#titles-from-headings

Copy link
Member

Choose a reason for hiding this comment

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

If this is centered, I think the last option is best with perhaps swapped lines for the status and description?

I'm not sure of the markup needed for this though.

Copy link
Member Author

Choose a reason for hiding this comment

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

@zimeg Thanks for sharing those links! 🙇🏻

Since using HTML can lead to parsing problems for some readers AND popular Golang projects use standard markdown, let's align with the Golang community and continue to use a #.

I think we could consider adding a header image that uses the Slack CLI Logo and text, to follow in the foot steps of other popular Golang projects:

Copy link
Contributor

@lukegalbraithrussell lukegalbraithrussell left a comment

Choose a reason for hiding this comment

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

+1 to william's small suggestion on centering the image

Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

LGTM! And I too am a fan of bringing a splash to this document 🎁

Do you notice that the logo is offset vertically a little bit?

@mwbrooks Incredible eye! I use Safari but to me this offset is within reasonable doubt:

CLI

Overall I'd lean toward not handling special cases in this formatting since this might not render as expected elsewhere. While I expected images to align, this might be something that's even changed later within the browser!

The discussion around centering the header and details is also something I'm interested in, but I notice other go projects do not follow that convention:

And aligning to this seems best to me, but no blocker.

The care for this page is so much appreciated overall 👾 ✨

Copy link
Member

Choose a reason for hiding this comment

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

❤️

@@ -1,4 +1,4 @@
# Slack CLI
# Slack CLI <img src="docs/static/img/slack-cli-logo.svg" alt="Slack CLI logo" style="height: 1em; max-width: 100%" />
Copy link
Member

Choose a reason for hiding this comment

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

📝 Adding onto this to prefer using # for title headings and following sections for standard parsings that some readers might use:

https://github.com/artempyanykh/marksman/blob/main/docs/features.md#titles-from-headings

@@ -1,4 +1,4 @@
# Slack CLI
# Slack CLI <img src="docs/static/img/slack-cli-logo.svg" alt="Slack CLI logo" style="height: 1em; max-width: 100%" />
Copy link
Member

Choose a reason for hiding this comment

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

If this is centered, I think the last option is best with perhaps swapped lines for the status and description?

I'm not sure of the markup needed for this though.

@@ -1,6 +1,5 @@
# Project build files
bin/
docs/
Copy link
Member

Choose a reason for hiding this comment

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

Removing this makes sense to me with the other documentation changes in progress! IIRC additional written pages - not autogen - might soon find this path.

We might want to consider either committing the autogen files or ignoring these here though. I lean towards ignoring since the build process for docs uses this command itself! Otherwise, we might want to fail CI if these files don't match whats expected...

No blocker for this PR though! These files are find to keep unstaged after docgen for now 📚 ✨

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that we may want to change the default path for docgen and/or .gitignore the files.

Since docgen is a hidden command, I think we can safely change the default directory with a patch or minor. Let's hold off until we move the public docs over, then we can decide what makes the most sense!

@mwbrooks mwbrooks merged commit 3d88dfe into main Apr 4, 2025
6 checks passed
@mwbrooks mwbrooks deleted the mbrooks-readme-logo branch April 4, 2025 18:42
@zimeg
Copy link
Member

zimeg commented Apr 11, 2025

While I expected images to align, this might be something that's even changed later within the browser!

📝 Huh this happened sooner than I had guessed... https://developer.mozilla.org/en-US/blog/h1-element-styles/

Default styles for h1 elements are changing

@mwbrooks
Copy link
Member Author

📝 Huh this happened sooner than I had guessed... https://developer.mozilla.org/en-US/blog/h1-element-styles/

Default styles for h1 elements are changing

So cool to see the default UA styles for online text changing before our eyes! Let's keep an eye on our README.md to make sure it continues to look sharp ✨

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

Labels

docs M-T: Documentation work only semver:patch Use on pull requests to describe the release version increment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants