-
-
Notifications
You must be signed in to change notification settings - Fork 273
Added new custom tags tip, warning and danger #314
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
Conversation
@benjagm Please review and let me know if any other changes required. |
Do these always need the title tab, e.g. the "Draft specific info"? Potentially we want to be able to omit that. |
It is already optional, if we dont pass the label prop it will be automatically omited. |
What's the markdown syntax for these? For example, if I want to use this in a blog post, how do I do that? (This was the original ask that inspired the issue.) The Chirpy syntax is > This text looks like a block quote, but the following line makes it tender in a styled box.
{: .prompt-info } My (non-web-developer) guess is that the content of the |
Looks like you can use the component name as HTML in the markdown. I believe the main site content and the blog posts are processed the same from markdown. website/pages/understanding-json-schema/basics.md Lines 122 to 127 in 350b997
|
That is right Ben. We have a set of those custom HTML tags and the intend of this issue is to create some more. |
@Relequestual that example helps, but it only shows a label. How is this new config value specified? I don't see that. Examples of usage would be great. |
Assuming that by "config value", you're assuming that you pass an attribute to specify the type of box being used, you'd be incorrect in that assumption, although reasonable. In the opening comment, they said they have added the tags Tip, Warning, and Danger. Literally, you can now use those as HTML tags in the markdown which will be converted to the component during compliation. Want to use a "Tip" box, use the HTML tag I know the code could be refactored to reduce duplication, but it does the job. @divyaxdv Did you want to have a go at refactoring this to reduce duplication? You'd want to do something similar as in this article I think. |
Sure, I will update it. |
I have created an Information component and is refactoring new tags using it . I have passed the imgSrc and bgcolor prop but for some reason bgcolor prop is not updating colors for tip, warning and danger. I have inspected the elements to check classname and they are fine but still colors are not getting updated. @Relequestual @gregsdennis @benjagm . |
You have done an amazing job @divyaxdv!! Huge thanks! I just pushed some changes to fix the issues with colours. I needed to try different things and finally I fixed it. I also added some other changes. Please, help let me know if now the issue is fixed. |
Thanks! @benjagm for your time and help. But I still dont see the colors being updated for tip, danger and warning . It is working fine for Infobox as mentioned. |
It is very Strange. I have checked multiple times and also with different browsers like chrome, firefox , edge but the issue persists.
Can you please confirm if I am using this in a right way and also can you please make sure that all the changes are pushed ? |
Hello @gregsdennis @Relequestual when you get a chance can you also kindly check on your local if the latest changes are working as expected. |
I'm not currently set up to run the site. |
Trying a different approach
You where right, that version was not working as expected. I am pushing another version adding all the code inside StyledMarkdown and not using the Information component, and this seems to work. The previous solution was cleaner but for any reason the styles didn't work. |
@divyaxdv How do you feel with the solution I pushed? |
This totally works, but I was still figuring how it can be done using the component(information). Until then I think we should merge this PR and it could be refactored later if we find a better solution. |
My concern with this is that if we merge now and new content is written that uses these features, when we refactor, we'll have to either remember to go everywhere and update the content or we'll have to refactor in a backwards compatible way. If we're happy to do that, sure, but we need to be aware that this is an outcome of merging now. |
The way we are trying to refactor will not effect the usage of these tags. Newer content will also use only the label props and other props like color and the icon, that is our design choice so these props will not be exposed for the users to change which will keep the usage consistent throughout the app even though we refactor. @gregsdennis What are your view ? @benjagm |
@gregsdennis @divyaxdv I think it is ok to merge this. The tagging will be the same with indepence of the implementation we choose. What I tink is worth is create a md file with the annotations we can use when writing markdown. |
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.
great work @divyaxdv !!
GitHub Issue: #239
Resolves: #239
Summary:
In this PR, I have added new custom tags namely Tip, Warning and Danger
Do you think resolving this issue might require an Architectural Decision Record (ADR)? (significant or noteworthy)
No