Skip to content

Conversation

sarahsanders-docker
Copy link
Contributor

@sarahsanders-docker sarahsanders-docker commented Jan 23, 2025

Description

  • Updated "Contribute" guide to reference summary bars, added instructions for using them
  • Removed introduced partial, this is now replaced by the summary bar requires value
  • Updated all files using introduced partial to use summary bar, updated YAML file w/ feature name and info

Related issues or tickets

ENGDOCS-2392

Reviews

  • Technical review
  • Editorial review

@sarahsanders-docker sarahsanders-docker requested review from a team and dvdksn January 23, 2025 22:52
@github-actions github-actions bot added area/engine Issue affects Docker engine/daemon area/compose Relates to docker-compose.yml spec or docker-compose binary area/build Relates to Dockerfiles or docker build command area/desktop Issue affects a desktop edition of Docker. E.g docker for mac area/security area/contrib Relates to the Docker style guide and contribution guidelines hugo Updates related to hugo labels Jan 23, 2025
Copy link

netlify bot commented Jan 23, 2025

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit fb65ae5
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/679a63508dca1100089276f3
😎 Deploy Preview https://deploy-preview-21887--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@aevesdocker
Copy link
Contributor

I'm actually not sure about replacing the introduced partial with the summary bar 🤔 (sorry, I know you've gone to all that effort in removing and swapping them)
Visually, I'm immediately drawn to the summary bar, rather than reading the text. Also, it's quite useful to have the link to the release notes in these circumstances. I think it looks much stronger if we use the summary bar just for the top of the page (where appropriate) and then the introduced partial for in-page items, but keen to hear what the rest of the team thinks

@dvdksn
Copy link
Contributor

dvdksn commented Jan 27, 2025

I'd much rather have just one way of showing this data. I thought we ended up with a shortcode for the summary bar because it allows us the flexibility to put it on a section-level? If we only ever want to render this component at the top of a page, then it probably shouldn't even be a shortcode. Pretty sure we discussed this when reviewing the original PR.

@sarahsanders-docker
Copy link
Contributor Author

I'd much rather have just one way of showing this data. I thought we ended up with a shortcode for the summary bar because it allows us the flexibility to put it on a section-level? If we only ever want to render this component at the top of a page, then it probably shouldn't even be a shortcode. Pretty sure we discussed this when reviewing the original PR.

I would also prefer to have one way of showing this data. I think if we introduce too many types of callouts or visual elements that represent a bunch of different things, we get into a territory that becomes difficult to maintain and be consistent with. Also as a user, the more consistent we are, the better I recognize and understand these elements over time.

For these visual elements, I would prefer keeping things limited to summary bars and callouts, and implementing them at the section level as needed. Also, these are implemented at the section level in other docs from the original PR.

@aevesdocker I can adjust the text shown in the Requires: section of the summary bar to include release notes. that is an easy fix and I can update the contribute guide to have that be standard when there are corresponding release notes

@sarahsanders-docker sarahsanders-docker added the status/do-not-merge Pull requests that are awaiting some event or decision before they can be merged. label Jan 27, 2025
@sarahsanders-docker
Copy link
Contributor Author

@aevesdocker @dvdksn I am able to add the release notes to the summary bar Requires: line by slightly adjusting the shortcode file. If this is a good compromise, I can push this change!
image

@aevesdocker
Copy link
Contributor

Looks like I'm outnumbered! Adding the release notes link LGTM, thanks @sarahsanders-docker

Copy link
Contributor

@dvdksn dvdksn left a comment

Choose a reason for hiding this comment

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

You can also remove these, they were used by the introduced.html shortcode

  • docs/i18n/en.yaml

    Lines 39 to 54 in ee59422

    ## component names
    buildx:
    other: Buildx
    buildkit:
    other: BuildKit
    engine:
    other: Docker Engine
    api:
    other: Engine API
    desktop:
    other: Docker Desktop
    compose:
    other: Docker Compose
    scout:
    other: Docker Scout
  • docs/hugo.yaml

    Lines 130 to 139 in ee59422

    # Minimum version thresholds (used together with the "introduced" shortcode
    # See layouts/shortcodes/introduced.html
    min_version_thresholds:
    buildx: "0.10.0"
    buildkit: "0.11.0"
    engine: "24.0.0"
    api: "1.41"
    desktop: "4.20.0"
    compose: "2.14.0"
    scout: "1.0.0"

@sarahsanders-docker sarahsanders-docker requested review from a team and dvdksn January 29, 2025 17:32
@sarahsanders-docker sarahsanders-docker merged commit 9f56d94 into docker:main Jan 30, 2025
14 checks passed
@sarahsanders-docker sarahsanders-docker deleted the summary-bar-nits branch March 12, 2025 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/build Relates to Dockerfiles or docker build command area/compose Relates to docker-compose.yml spec or docker-compose binary area/config area/contrib Relates to the Docker style guide and contribution guidelines area/desktop Issue affects a desktop edition of Docker. E.g docker for mac area/engine Issue affects Docker engine/daemon area/security hugo Updates related to hugo status/do-not-merge Pull requests that are awaiting some event or decision before they can be merged. status/review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants