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

feat(compiler): customize readme mermaid diagram colors #5980

Merged
merged 8 commits into from
Sep 11, 2024

Conversation

Tardigrada777
Copy link
Contributor

@Tardigrada777 Tardigrada777 commented Sep 7, 2024

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:

export const config: Config {
  docs: {
    markdown: {
      targetComponent: {
        background: '#f0f0f0',
        textColor: '#f2f3f4',
      },
    },
  },
  // other options
}

Does this introduce a breaking change?

  • Yes
  • No

Testing

Just specify these colors in the config and run npm run build. Newly generated readme.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".

Apply colors from the `docs.markdown.targetComponent` to
the my-component node in the mermaid diagram.

fixes: ionic-team#2876
Copy link
Member

@tanner-reits tanner-reits left a 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!

src/compiler/docs/readme/constants.ts Outdated Show resolved Hide resolved
src/compiler/docs/readme/markdown-dependencies.ts Outdated Show resolved Hide resolved
src/declarations/stencil-public-compiler.ts Outdated Show resolved Hide resolved
Apply colors from the `docs.markdown.targetComponent` to
the my-component node in the mermaid diagram.

fixes: ionic-team#2876
Comment on lines 37 to 50
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.');
}
Copy link
Member

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 😞

Copy link
Contributor Author

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 🙂

Copy link
Member

@tanner-reits tanner-reits left a 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!

Changes-On-2de6d4.patch

@Tardigrada777
Copy link
Contributor Author

@tanner-reits I think that's totally fine to pass the logger instance to the validate function explicitly. Though it's weird, I saw in other files that logger is part of the validated config object. May be I'm wrong, at least in validate-config.ts:92 I see that the config is being created like that: bootstrapConfig.logger || config.logger || createNodeLogger(). May be that's the reason why logger is not in the config object.

If you can apply the patch, please, go ahead. If you need my assistance, please, let me know.

@tanner-reits tanner-reits changed the title feat(compiler): add docs option to the config feat(compiler): customize readme mermaid diagram colors Sep 11, 2024
@tanner-reits tanner-reits added this pull request to the merge queue Sep 11, 2024
Merged via the queue into ionic-team:main with commit 9ca8951 Sep 11, 2024
88 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make mermaid diagram colors configurable
2 participants