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: add support for markdown in toggle descriptions #6453

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ivarconr
Copy link
Member

@ivarconr ivarconr commented Mar 6, 2024

This is a small PoC on supporting limited markdown in the feature flag descriptions. I think it make sense to only support a very basic set of markdown formatting initially, as it can easily mess up the UI to support to much. Current support is for
the following allowed html elements: "a", "b", "p", "strong" and "em", anything else is automatically stripped away.

You enter Markdown when defining a toggle:
image

Markdown is used on the feature toggle view:
image

Markdown is converted to plain text in the toggle list:
image

Markdown is used in the html tooltip:
image

Concerns:

  • I am concerned with how this implementation will try to strip the description for all toggles at render time. I am unsure of the performance of this.
  • Can we loose control on the rendering by supporting markdown?

Other possible solutions.

  • We could store a plain text description and a markdown formatted version when we create / update a feature toggle. This would remove some of the rendering cost on the UI, but would require changes in the response format in the API and lead to more data being exchanged.

Closes #6399

Copy link

vercel bot commented Mar 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview Mar 6, 2024 5:20pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Mar 6, 2024 5:20pm

@ivarconr ivarconr marked this pull request as draft March 6, 2024 17:21
Copy link
Member

@nunogois nunogois left a comment

Choose a reason for hiding this comment

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

This feels like a reasonable approach to me and if we detect any issues it's good to know that it's behind a flag 👍

"development": [
"last 1 chrome version",
"last 1 firefox version",
"last 1 safari version"
]
},
"dependencies": {
"remove-markdown": "^0.5.0"
Copy link
Member

Choose a reason for hiding this comment

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

Due to the way vite bundles dependencies, and for consistency sake, I think this belongs in devDependencies instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this was a lazy mistake by me.

@@ -12,6 +12,10 @@ import {
StyledDescription,
} from './LinkCell.styles';

//@ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

Just curious - Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

because remove-markdown does not have typescript definitions. I should add one instead of ignoring, just did not want to do it for the PoC.

@@ -26,23 +30,25 @@ export const LinkCell: React.FC<ILinkCellProps> = ({
subtitle,
children,
}) => {
const subTitleClean = removeMd(subtitle);
Copy link
Member

Choose a reason for hiding this comment

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

I think this casing is more consistent, since it matches the original name.

Alternatively, if it's more readable, we could use something like plainSubtitle or plainTextSubtitle.

Suggested change
const subTitleClean = removeMd(subtitle);
const subtitleClean = removeMd(subtitle);

@nunogois
Copy link
Member

nunogois commented Mar 6, 2024

I also have some concerns about the links styling inside the description panel in the feature view, since both are purple currently.

Maybe @nicolaesocaciu can help with a suggestion that has better contrast.

@nicolaesocaciu
Copy link
Contributor

When we have links on a purple background they should be "white" similar with the links in the purple sidepanel

@ivarconr
Copy link
Member Author

When we have links on a purple background they should be "white" similar with the links in the purple sidepanel

Agreed. I did not consider any styles yet, but this needs to be addressed if we move forward with markdown and link support in feature flag descriptions.

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

Successfully merging this pull request may close these issues.

feat: Markdown support in flag description
3 participants