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

Add support for adding status label component within documentation page headings #5353

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

Conversation

pastelcyborg
Copy link
Contributor

@pastelcyborg pastelcyborg commented Sep 17, 2024

Done

  • Add support for adding a status label component within a documentation page heading
  • Add new vf-u-align--middle utility for vertically aligning inline elements
  • Update table of contents JS script to support new status label addition

Fixes WD-14923
Fixes #5349

QA

  • Open equal height row docs (and any other docs pages with recent changes)
  • Witness new status label within relevant headings

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix release (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

@webteam-app
Copy link

@pastelcyborg pastelcyborg changed the title 5349 status label within docs Add support for adding status label component within documentation page headings Sep 17, 2024
@pastelcyborg pastelcyborg marked this pull request as ready for review September 17, 2024 20:50
@petesfrench
Copy link
Contributor

Thank you for jumping on this so quickly! This is a huge QOL improvement.

@bartaz
Copy link
Member

bartaz commented Sep 18, 2024

We used to do it in the past (label was put below heading) but it was kind of pain to maintain, because you would have to remember to remove those in next non-patch releases.
So without being able to do it automatically it's just gonna be the same mundane process again (or we will likely just forget to take them off).

Another reason that we used to put label below heading is to avoid these issues of having label as part of heading text. Both it adds itself to the id (which you workaround here, but still it seems bit hacky and fiddly), but also I have some accessibility doubts if it makes sense for this label text to actually be in the header (accessibility tree, screen readers, etc).

So while I agree it would be a significant improvement, we should maybe take a look how to do it properly, to avoid the mistakes of the past.

@petesfrench
Copy link
Contributor

I would argue that the status label in the side-navigation is essentially useless without some kind of indication within the documentation. As currently the only way to know exactly what changed is the check the change-log.

If you are to automate it, you can add it after the heading IDs are generated avoiding any nefarious coding practices. And as for accessibility concerns, this is nothing here that can't be addressed with the use of ARIA.

@pastelcyborg
Copy link
Contributor Author

After speaking in a meeting earlier today, we've decided to submit this as-is, as it's still a significant QoL improvement even without automation and such. I'll add a new process step to our docs to remind contributors to remove any outdated status labels before a new release.

We'll be exploring ways to better automate this with our new architecture projects.

@jmuzina
Copy link
Member

jmuzina commented Sep 19, 2024

4.16.0 has already been released (and no PR that bumps the version has been merged), so this should bump the version to 4.17.0.

guides/pull-requests.md Outdated Show resolved Hide resolved
@mixin vf-u-align--bottom {
.u-align--bottom {
margin-top: auto !important;
}
}

// Vertically align inline elements
@mixin vf-u-align--middle {
.u-align--middle {
Copy link
Member

Choose a reason for hiding this comment

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

It may be a bit confusing that we have u-align--bottom (that handles aligning with flexbox using margin) and u-align--middle that works in completely different context using completely different props (vertical-align).

I'd argue that the new u-align--middle is actually less confusing than the existing u-align--bottom (that doesn't seem to suggest by its name what context it's meant for).

But not sure how best to address this inconsistency.

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

Successfully merging this pull request may close these issues.

Add status label within documentation, not just navigation
5 participants