-
Notifications
You must be signed in to change notification settings - Fork 166
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
base: main
Are you sure you want to change the base?
Add support for adding status label component within documentation page headings #5353
Conversation
Thank you for jumping on this so quickly! This is a huge QOL improvement. |
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. 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. |
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. |
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. |
dc15111
to
c45a88c
Compare
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. |
@mixin vf-u-align--bottom { | ||
.u-align--bottom { | ||
margin-top: auto !important; | ||
} | ||
} | ||
|
||
// Vertically align inline elements | ||
@mixin vf-u-align--middle { | ||
.u-align--middle { |
There was a problem hiding this comment.
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.
Done
vf-u-align--middle
utility for vertically aligning inline elementsFixes WD-14923
Fixes #5349
QA
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:
Feature 🎁
,Breaking Change 💣
,Bug 🐛
,Documentation 📝
,Maintenance 🔨
.package.json
should be updated relative to the most recent release, following semver convention: