🎁 Use commonDirectiveOptions in all directives
#1650
Conversation
🦋 Changeset detectedLatest commit: 826df5b The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@fwkoch can you help clarify the changeset semver policy? I'm not clear for mystmd when we can make a minor bump. |
|
Requires jupyter-book/myst-theme#500 to see some of the improvements. |
|
I put my thoughts about the semver policy here: #1658 - nothing official in that yet, just my opinions! |
There was a problem hiding this comment.
This is really nice for adding class to all node types, it adds that feature while simplifying the code.
My only hesitation is that it also adds enumerated/enumerator to all the node types. To me, this feels slightly less general than class, which is just a transparent string that themes may or may not support...
Enumeration gets picked up by the enumerate transforms and impacts the mdast processing. Testing a little bit, it seems ok... good actually... I can now enumerate many things that I could not before, and it seems to just work. However, it makes the scope of this PR bigger than it might seem at first - it doesn't just add some simple properties to directives, it enriches (hopefully without breaking) fairly significant myst functionality.
Anyway, after going through all this, I'm still approving - I don't actually think there is a problem! Mostly just had to wrap my head around the implications of enumerated.
| @@ -11,11 +11,7 @@ export const asideDirective: DirectiveSpec = { | |||
| doc: 'An optional title', | |||
| }, | |||
| options: { | |||
| ...labelDirectiveOption('aside'), | |||
| // TODO: Add enumeration in future | |||
There was a problem hiding this comment.
This gets added to the node now too!
|
Yes @fwkoch, and helpful of you to explicitly surface that change --I made the same judgment that it feels like we should be able to identify and reference most (all) directives, but that isn't a small conceptual change by any means. |
Co-authored-by: Franklin Koch <[email protected]>
Co-authored-by: Franklin Koch <[email protected]>
|
@fwkoch thanks for your review! Really good catch on the I've also reworked the docs to use If you are OK with these changes, please merge the PR! |
|
|
||
| Removes the directive from the final output. | ||
| :::{myst:directive} exercise |
There was a problem hiding this comment.
Great to be using this here now! Thank you!!
|
The last thing I did was add |
|
@fwkoch thank you for catching those mistakes. Fab. |
This PR tackles #1657. We will need an upstream
myst-themePR to ensure that these are actually set in the DOM.I omitted the documentation directives
{myst:role}et al. I didn't think they needed it.