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

Support for applying bold and italics on absolute links #1718

Closed
wants to merge 8 commits into from

Conversation

vivinkrishna-ni
Copy link
Contributor

Pull Request

🤨 Rationale

Earlier, we are not supporting the application of bold and italics formatting to the absolute links as the markdown serializer for autolink has the limitation of converting formatting tags (<strong> and/or <em>) within anchor. For example, if the DOM is like the below,

<a href="https://nimble.ni.dev"><strong>https://nimble.ni.dev</strong></a>

The markdown output looks like the below as we support only autolink from CommonMark and the link (Point 5 checklist in #1527),

<**https://nimble.ni.dev**>

which is not a valid markdown format.

Also fixes a issue in mention support for comments feature. See: Technical Debt 2603501: FE | Bold and Italics not working if the text is selected together with mention node

👩‍💻 Implementation

  • Changed the prioritization of rendering the links within the editor and viewer. So that link will be rendered within other formatting tags like <strong> for bold and <em> for italics.
    • In editor, lowered the priority of links compared to other marks like bold and italics.
    • In viewer, added the link mark after the bold and italics so that even in the markdown-parser schema the priority of rendering the link is lower compared to bold and italics.
  • Removed excludes="_" as the default setting allows multiple marks.
  • Set the nimble-anchor font style to unset so that if the nimble-anchor is within the <strong> and <em> will take effect in the UI.

🧪 Testing

  • Update the existing tests where the combination of bold and italics applied on links
  • Written a new test to verify bold and italics after the mention node is rendered as expected
  • Updated the example markdown string to test the bold and italics for links in Chromatic UI

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@vivinkrishna-ni vivinkrishna-ni marked this pull request as draft December 22, 2023 12:26
@vivinkrishna-ni
Copy link
Contributor Author

@m-akinc Could you please buddy review this PR?

@vivinkrishna-ni vivinkrishna-ni marked this pull request as ready for review January 3, 2024 04:26
@@ -48,6 +48,10 @@ export const styles = css`
display: none;
}

nimble-anchor {
font: unset;
Copy link
Member

Choose a reason for hiding this comment

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

It's almost never worth using the value unset. Folks generally don't know how it behaves.
In this case it is equivalent to setting it to font: inherit since font is an inheritable property.
But I think overriding the entire font isn't the correct approach. We specifically want to let the font-weight and font-style through and let the anchor keep it's other font styling so we should just do those.

Suggested change
font: unset;
font-weight: inherit;
font-style: inherit;

@@ -0,0 +1,7 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

This change lets me make the middle of a link bold:
image

Which results in the following markdown:
image

Which doesn't look right to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an issue with the current approach which requires complete implementation change in the PR. I am moving this PR back to draft and once found a proper fix, will make it ready for review, else will abandon this PR.

@@ -1 +1 @@
export const richTextMarkdownString = 'Supported rich text formatting options:\n1. **Bold**\n2. *Italics*\n3. Numbered lists\n 1. Option 1\n 2. Option 2\n4. Bulleted lists\n * Option 1\n * Option 2\n5. Absolute link: <https://nimble.ni.dev/>\n 6. @mention:\n 1. User pattern: <user:1>\n 2. HTTPS pattern: <https://user/2>';
export const richTextMarkdownString = 'Supported rich text formatting options:\n1. **Bold**\n2. *Italics*\n3. Numbered lists\n 1. Option 1\n 2. Option 2\n4. Bulleted lists\n * Option 1\n * Option 2\n5. Absolute link: ***<https://nimble.ni.dev/>***\n 6. @mention:\n 1. User pattern: <user:1>\n 2. HTTPS pattern: <https://user/2>';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a good way to test this change. This test string should render each construct in its default configuration since it's used by user-facing examples and by tests that cover behavior of common cases.

Instead we should have some dedicated matrix tests that ensure absolute links and @ mentions render correctly when configured to be bold, italics, and both.

@@ -0,0 +1,7 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing odd behavior when moving the text cursor through a mention that's got bold and italic styling. The part of the mention before the cursor loses its styling while the part after it retains its styling. Perhaps this is expected behavior for our current implementation or isn't worth fixing, but it felt odd to me. It looks like this behavior existed before this change to some degree too.

Screen.Recording.2024-01-03.at.4.51.03.PM.mov

@vivinkrishna-ni vivinkrishna-ni marked this pull request as draft January 4, 2024 10:30
@vivinkrishna-ni
Copy link
Contributor Author

This fix may not be achieved with the current implementation changes that are proposed in this PR and also per the requirements in supporting only absolute links. Therefore, I am closing the PR, and once the hyperlink support is enabled in rich text components the issue mentioned in the description will be fixed (part of #1527)

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.

5 participants