-
-
Notifications
You must be signed in to change notification settings - Fork 730
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
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 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" |
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.
Due to the way vite bundles dependencies, and for consistency sake, I think this belongs in devDependencies
instead.
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.
yes, this was a lazy mistake by me.
@@ -12,6 +12,10 @@ import { | |||
StyledDescription, | |||
} from './LinkCell.styles'; | |||
|
|||
//@ts-ignore |
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.
Just curious - Why is this needed?
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.
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); |
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.
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
.
const subTitleClean = removeMd(subtitle); | |
const subtitleClean = removeMd(subtitle); |
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. |
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. |
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:
Markdown is used on the feature toggle view:
Markdown is converted to plain text in the toggle list:
Markdown is used in the html tooltip:
Concerns:
Other possible solutions.
Closes #6399