-
Notifications
You must be signed in to change notification settings - Fork 789
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
feat(compiler): customize readme mermaid diagram colors #5980
feat(compiler): customize readme mermaid diagram colors #5980
Conversation
Apply colors from the `docs.markdown.targetComponent` to the my-component node in the mermaid diagram. fixes: ionic-team#2876
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.
This looks good! Only additional ask I have would be to add some unit tests for the depsToMarkdown
method. I know there aren't any test files/cases for that right now, so let me know if you have questions (don't worry about testing all functionality, just the bits you changed). If you don't have time/capacity, I'm happy to hop in an take care of that!
Apply colors from the `docs.markdown.targetComponent` to the my-component node in the mermaid diagram. fixes: ionic-team#2876
4261d8a
to
b5a10f3
Compare
const { background: defaultBackground, textColor: defaultTextColor } = DEFAULT_TARGET_COMPONENT_STYLES; | ||
|
||
let { background = defaultBackground, textColor = defaultTextColor } = | ||
config.docs?.markdown?.targetComponent ?? DEFAULT_TARGET_COMPONENT_STYLES; | ||
|
||
if (!isHexColor(background)) { | ||
background = defaultBackground; | ||
config.logger.warn('Default value for docs.markdown.targetComponent.background is being used.'); | ||
} | ||
|
||
if (!isHexColor(textColor)) { | ||
textColor = defaultTextColor; | ||
config.logger.warn('Default value for docs.markdown.targetComponent.textColor is being used.'); | ||
} |
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.
Alright, I may have mislead you here, so I apologize. When testing the warning I realized that this method executes for every component with dependencies, so it's a bit annoying that the warning logs for every instance. So, that made me realize we should probably move this logic to the validateConfig method (and update appropriate tests). That way, by this point, we know that the value from the config is valid and the warning will only log once per build.
Again, very sorry I didn't catch this on the first review 😞
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.
No prob @tanner-reits
You are right, I didn't run the build
command after the last changes... I could catch it too.
I'll have a look 🙂
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.
@Tardigrada777 This is looking really close! Ran into one build issue testing things out related to the logger not being defined on the config that gets passed to the validateDocs
function.
Made a tiny patch (attached to this comment) that fixes that issue and updates the logged message slightly. Check it out and let me know what you think!
@tanner-reits I think that's totally fine to pass the If you can apply the patch, please, go ahead. If you need my assistance, please, let me know. |
Apply colors from the
docs.markdown.targetComponent
to the my-component node in the mermaid diagram.fixes: #2876
What is the current behavior?
Currently, the background and text colors of the target node (my-component) in the Mermaid diagram are hardcoded, preventing users from customizing them.
GitHub Issue Number: #2876
What is the new behavior?
Users can now specify the background and text color of the target component in the 'docs' option within the Config. The compiler will then pick up these values and apply them to the generated Mermaid diagram.
Documentation
Users can specify colors in config:
Does this introduce a breaking change?
Testing
Just specify these colors in the
config
and runnpm run build
. Newly generatedreadme.md
files will contain Mermaid diagram with specified colors for the target component.Other information
I also added "run" to the one of the commands in CONTRIBUTING.md. There was a typo saying: "run npm install.jest".