From 3c8627b3e1fb7ef9551706a8fad38f34625032ea Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Wed, 27 Mar 2024 18:05:46 -0500 Subject: [PATCH 01/10] Fix a couple issues with anchor styling --- .../nimble-components/src/anchor/styles.ts | 129 ++++++++---------- .../src/theme-provider/design-tokens.ts | 2 +- 2 files changed, 61 insertions(+), 70 deletions(-) diff --git a/packages/nimble-components/src/anchor/styles.ts b/packages/nimble-components/src/anchor/styles.ts index 3e5ed018a9..ae1d94bda0 100644 --- a/packages/nimble-components/src/anchor/styles.ts +++ b/packages/nimble-components/src/anchor/styles.ts @@ -2,102 +2,93 @@ import { css } from '@microsoft/fast-element'; import { display } from '@microsoft/fast-foundation'; import { focusVisible } from '../utilities/style/focus'; import { - linkActiveDisabledFontColor, linkActiveFontColor, linkActiveProminentFontColor, linkDisabledFontColor, linkFont, linkFontColor, - linkProminentDisabledFontColor, linkProminentFontColor } from '../theme-provider/design-tokens'; export const styles = css` - ${display('inline')} + @layer base, hover, focusVisible, active, disabled; - :host { - box-sizing: border-box; - font: ${linkFont}; - } - - .top-container { - display: contents; - } - - [part='start'] { - display: none; - } - - .control { - color: ${linkFontColor}; - text-decoration: underline; - } - - .control${focusVisible} { - display: inline; - outline: none; - box-shadow: inset 0px -1px ${linkFontColor}; - } - - .control:active { - color: ${linkActiveFontColor}; - text-decoration: underline; - } + @layer base { + ${display('inline')} - .control${focusVisible}:active { - box-shadow: inset 0px -1px ${linkActiveFontColor}; - } + :host { + box-sizing: border-box; + font: ${linkFont}; + } - .control:not(:any-link) { - color: ${linkDisabledFontColor}; - text-decoration: none; - } + .top-container { + display: contents; + } - .control:not(:any-link):active { - color: ${linkActiveDisabledFontColor}; - } + [part='start'] { + display: none; + } - :host([underline-hidden]) .control { - text-decoration: none; - } + .control { + color: ${linkFontColor}; + text-decoration: underline; + } - :host([underline-hidden]) .control:hover { - text-decoration: underline; - } + :host([underline-hidden]) .control { + text-decoration: none; + } - :host([underline-hidden]) .control${focusVisible} { - text-decoration: underline; - } + :host([appearance='prominent']) .control { + color: ${linkProminentFontColor}; + } - :host([underline-hidden]) .control:not(:any-link) { - text-decoration: none; - } + .content { + pointer-events: none; + } - :host([appearance='prominent']) .control { - color: ${linkProminentFontColor}; + [part='end'] { + display: none; + } } - :host([appearance='prominent']) .control:active { - color: ${linkActiveProminentFontColor}; + @layer hover { + .control:any-link:hover { + text-decoration: underline; + } } - :host([appearance='prominent']) .control${focusVisible} { - box-shadow: inset 0px -1px ${linkProminentFontColor}; + @layer focusVisible { + .control${focusVisible} { + outline: none; + box-shadow: inset 0px -1px; + text-decoration: underline; + } } - :host([appearance='prominent']) .control${focusVisible}:active { - box-shadow: inset 0px -1px ${linkActiveProminentFontColor}; - } + @layer active { + .control:active { + color: ${linkActiveFontColor}; + text-decoration: underline; + box-shadow: none; + } - :host([appearance='prominent']) .control:not(:any-link) { - color: ${linkProminentDisabledFontColor}; - } + :host([appearance='prominent']) .control:active { + color: ${linkActiveProminentFontColor}; + } - .content { - pointer-events: none; + ${ + '' /* It's probably cleaner to add :not(active) to the selector in the 'active' layer (rather + than resetting text-decoration back to the value from the 'default' layer), but + there's a bug in storybook-addon-pseudo-states where it doesn't handle :not() properly. */ + } + :host([underline-hidden]) .control:active { + text-decoration: none; + } } - [part='end'] { - display: none; + @layer disabled { + .control:not(:any-link) { + color: ${linkDisabledFontColor}; + } } `; diff --git a/packages/nimble-components/src/theme-provider/design-tokens.ts b/packages/nimble-components/src/theme-provider/design-tokens.ts index dcb6b0fb05..3d63c10539 100644 --- a/packages/nimble-components/src/theme-provider/design-tokens.ts +++ b/packages/nimble-components/src/theme-provider/design-tokens.ts @@ -588,7 +588,7 @@ export const [ element, DigitalGreenLight, DigitalGreenLight, - PowerGreen + hexToRgbaCssColor(White, 0.6) ), (element: HTMLElement) => hexToRgbaCssColor(getDefaultFontColorForTheme(element), 0.3), LinkLightUiFamily, From fed65fa3fab5c6706dd5535f633a1c835c59078c Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Wed, 27 Mar 2024 18:06:01 -0500 Subject: [PATCH 02/10] Change files --- ...le-components-c92408f2-14ec-4254-9aac-0babfa7fbbd1.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@ni-nimble-components-c92408f2-14ec-4254-9aac-0babfa7fbbd1.json diff --git a/change/@ni-nimble-components-c92408f2-14ec-4254-9aac-0babfa7fbbd1.json b/change/@ni-nimble-components-c92408f2-14ec-4254-9aac-0babfa7fbbd1.json new file mode 100644 index 0000000000..39d134519e --- /dev/null +++ b/change/@ni-nimble-components-c92408f2-14ec-4254-9aac-0babfa7fbbd1.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Fix a couple issues with anchor styling", + "packageName": "@ni/nimble-components", + "email": "7282195+m-akinc@users.noreply.github.com", + "dependentChangeType": "patch" +} From fce00df32afbafb6237cd5d9280bb7bf1fcc1b59 Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 28 Mar 2024 19:10:47 -0500 Subject: [PATCH 03/10] Remove comment --- packages/nimble-components/src/anchor/styles.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/nimble-components/src/anchor/styles.ts b/packages/nimble-components/src/anchor/styles.ts index ae1d94bda0..fcb3959759 100644 --- a/packages/nimble-components/src/anchor/styles.ts +++ b/packages/nimble-components/src/anchor/styles.ts @@ -76,11 +76,6 @@ export const styles = css` color: ${linkActiveProminentFontColor}; } - ${ - '' /* It's probably cleaner to add :not(active) to the selector in the 'active' layer (rather - than resetting text-decoration back to the value from the 'default' layer), but - there's a bug in storybook-addon-pseudo-states where it doesn't handle :not() properly. */ - } :host([underline-hidden]) .control:active { text-decoration: none; } From 2b49b01093e856ea7fde1e65c155bc86d8e0a035 Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Mon, 1 Apr 2024 14:34:43 -0500 Subject: [PATCH 04/10] Tweak breadcrumb and rich-text viewer link styling --- .../src/breadcrumb-item/styles.ts | 2 +- .../src/rich-text/viewer/styles.ts | 14 -------------- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/packages/nimble-components/src/breadcrumb-item/styles.ts b/packages/nimble-components/src/breadcrumb-item/styles.ts index 091265edf3..e59a1ffb4e 100644 --- a/packages/nimble-components/src/breadcrumb-item/styles.ts +++ b/packages/nimble-components/src/breadcrumb-item/styles.ts @@ -54,7 +54,7 @@ export const styles = css` .control:active { color: var(--ni-private-breadcrumb-link-active-font-color); - text-decoration: underline; + text-decoration: none; } [part='start'] { diff --git a/packages/nimble-components/src/rich-text/viewer/styles.ts b/packages/nimble-components/src/rich-text/viewer/styles.ts index c4644b2928..57cb92bf06 100644 --- a/packages/nimble-components/src/rich-text/viewer/styles.ts +++ b/packages/nimble-components/src/rich-text/viewer/styles.ts @@ -45,18 +45,4 @@ export const styles = css` li > p:empty { display: none; } - - ${ - /** - * When an absolute link is not HTTPS/HTTP, the anchor tag renders without an `href`, appearing as plain text. - * However, the `nimble-anchor` displays differently in color when the `href` attribute is absent. - * To ensure a consistent appearance, the font color is forced to the default link color regardless of the `href` - * attribute's presence. - * - * See models/markdown-parser.ts where link elements are emitted for more info. - */ '' - } - nimble-anchor::part(control) { - color: ${linkFontColor}; - } `; From 959686e1227470989c7c06858001e35bbdfd8dda Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Mon, 1 Apr 2024 14:47:35 -0500 Subject: [PATCH 05/10] Lint fix --- packages/nimble-components/src/rich-text/viewer/styles.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/nimble-components/src/rich-text/viewer/styles.ts b/packages/nimble-components/src/rich-text/viewer/styles.ts index 57cb92bf06..0ffe474bab 100644 --- a/packages/nimble-components/src/rich-text/viewer/styles.ts +++ b/packages/nimble-components/src/rich-text/viewer/styles.ts @@ -1,10 +1,6 @@ import { css } from '@microsoft/fast-element'; import { display } from '@microsoft/fast-foundation'; -import { - bodyFont, - bodyFontColor, - linkFontColor -} from '../../theme-provider/design-tokens'; +import { bodyFont, bodyFontColor } from '../../theme-provider/design-tokens'; export const styles = css` ${display('flex')} From 54cfd049010c57ee89e3379f45ec3d305354c4b9 Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Mon, 1 Apr 2024 15:48:55 -0500 Subject: [PATCH 06/10] Revert change to rich-text viewer link styling --- .../src/rich-text/viewer/styles.ts | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/nimble-components/src/rich-text/viewer/styles.ts b/packages/nimble-components/src/rich-text/viewer/styles.ts index 0ffe474bab..c4644b2928 100644 --- a/packages/nimble-components/src/rich-text/viewer/styles.ts +++ b/packages/nimble-components/src/rich-text/viewer/styles.ts @@ -1,6 +1,10 @@ import { css } from '@microsoft/fast-element'; import { display } from '@microsoft/fast-foundation'; -import { bodyFont, bodyFontColor } from '../../theme-provider/design-tokens'; +import { + bodyFont, + bodyFontColor, + linkFontColor +} from '../../theme-provider/design-tokens'; export const styles = css` ${display('flex')} @@ -41,4 +45,18 @@ export const styles = css` li > p:empty { display: none; } + + ${ + /** + * When an absolute link is not HTTPS/HTTP, the anchor tag renders without an `href`, appearing as plain text. + * However, the `nimble-anchor` displays differently in color when the `href` attribute is absent. + * To ensure a consistent appearance, the font color is forced to the default link color regardless of the `href` + * attribute's presence. + * + * See models/markdown-parser.ts where link elements are emitted for more info. + */ '' + } + nimble-anchor::part(control) { + color: ${linkFontColor}; + } `; From 5a4a5f735408ea963d5836b22b90e4193cacbfba Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Mon, 1 Apr 2024 17:11:20 -0500 Subject: [PATCH 07/10] Improve comment and remove underline from non-href absolute links --- .../src/rich-text/models/markdown-parser.ts | 18 ++++++++++++++++-- .../src/rich-text/viewer/styles.ts | 11 +++++++---- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/packages/nimble-components/src/rich-text/models/markdown-parser.ts b/packages/nimble-components/src/rich-text/models/markdown-parser.ts index 6b211d3dfa..b8163257ec 100644 --- a/packages/nimble-components/src/rich-text/models/markdown-parser.ts +++ b/packages/nimble-components/src/rich-text/models/markdown-parser.ts @@ -148,12 +148,22 @@ export class RichTextMarkdownParser { * scheme and no matching mention pattern will be rendered as plain text (anchor with no href). * With this, the user can click the links only when the scheme is HTTP/HTTPS */ - href: /^https?:\/\//i.test(href) ? href : null, + href: RichTextMarkdownParser.startsWithHttpOrHttps( + href + ) + ? href + : null, rel: node.attrs.rel as Attr, // Adding `class` here is a workaround to render two mentions without a whitespace as display names // This attribute can be removed when the below issue is resolved // https://github.com/ni/nimble/issues/1707 - class: href + class: href, + 'underline-hidden': + RichTextMarkdownParser.startsWithHttpOrHttps( + href + ) + ? null + : true } ]; } @@ -177,4 +187,8 @@ export class RichTextMarkdownParser { RichTextMarkdownParser.mentionConfigs = undefined; RichTextMarkdownParser.mentionedHrefs.clear(); } + + private static startsWithHttpOrHttps(href: string): boolean { + return /^https?:\/\//i.test(href); + } } diff --git a/packages/nimble-components/src/rich-text/viewer/styles.ts b/packages/nimble-components/src/rich-text/viewer/styles.ts index c4644b2928..4deaf33e3d 100644 --- a/packages/nimble-components/src/rich-text/viewer/styles.ts +++ b/packages/nimble-components/src/rich-text/viewer/styles.ts @@ -48,10 +48,13 @@ export const styles = css` ${ /** - * When an absolute link is not HTTPS/HTTP, the anchor tag renders without an `href`, appearing as plain text. - * However, the `nimble-anchor` displays differently in color when the `href` attribute is absent. - * To ensure a consistent appearance, the font color is forced to the default link color regardless of the `href` - * attribute's presence. + * In the rich-text viewer, an absolute link renders as a native anchor, not a `nimble-anchor`. When such a link + * is not HTTPS/HTTP, the anchor renders without an `href`, appearing as plain text. + * However, in the rich-text viewer, absolute links are rendered as `nimble-anchor`s, and they do not look like + * plain text when the `href` attribute is absent. They have a "disabled" color and may have an underline. + * To ensure a consistent appearance between the editor and viewer, we force the font color to the default link/ + * plain text color regardless of the `href` attribute's presence. To remove the underline, the markdown parser + * emits an `underline-hidden` attribute when the `href` attribute is absent. * * See models/markdown-parser.ts where link elements are emitted for more info. */ '' From 1192f159b52fbdbb1d9a6b86a67d3ae362b56cdd Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Mon, 1 Apr 2024 17:18:32 -0500 Subject: [PATCH 08/10] Add to tests --- .../src/rich-text/models/tests/markdown-parser.spec.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/nimble-components/src/rich-text/models/tests/markdown-parser.spec.ts b/packages/nimble-components/src/rich-text/models/tests/markdown-parser.spec.ts index 45819d75f2..59da86a46a 100644 --- a/packages/nimble-components/src/rich-text/models/tests/markdown-parser.spec.ts +++ b/packages/nimble-components/src/rich-text/models/tests/markdown-parser.spec.ts @@ -564,7 +564,7 @@ describe('Markdown parser', () => { ] as const; parameterizeSpec(differentProtocolLinks, (spec, name) => { spec( - `string "${name}" renders within nimble-anchor without 'href' attribute`, + `string "${name}" renders within nimble-anchor without 'href' attribute and with 'underline-hidden'`, () => { const doc = RichTextMarkdownParser.parseMarkdownToDOM( name @@ -581,6 +581,12 @@ describe('Markdown parser', () => { expect( lastChildElementHasAttribute('href', doc) ).toBeFalse(); + expect( + lastChildElementHasAttribute( + 'underline-hidden', + doc + ) + ).toBeTrue(); } ); }); From 2e8ae5ec9acd28a7a54065eb498eae41cf05c5dc Mon Sep 17 00:00:00 2001 From: m-akinc <7282195+m-akinc@users.noreply.github.com> Date: Wed, 10 Apr 2024 12:04:29 -0500 Subject: [PATCH 09/10] Update packages/nimble-components/src/rich-text/viewer/styles.ts Co-authored-by: Milan Raj --- packages/nimble-components/src/rich-text/viewer/styles.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nimble-components/src/rich-text/viewer/styles.ts b/packages/nimble-components/src/rich-text/viewer/styles.ts index 4deaf33e3d..240f012af1 100644 --- a/packages/nimble-components/src/rich-text/viewer/styles.ts +++ b/packages/nimble-components/src/rich-text/viewer/styles.ts @@ -48,7 +48,7 @@ export const styles = css` ${ /** - * In the rich-text viewer, an absolute link renders as a native anchor, not a `nimble-anchor`. When such a link + * In the rich-text editor, an absolute link renders as a native anchor, not a `nimble-anchor`. When such a link * is not HTTPS/HTTP, the anchor renders without an `href`, appearing as plain text. * However, in the rich-text viewer, absolute links are rendered as `nimble-anchor`s, and they do not look like * plain text when the `href` attribute is absent. They have a "disabled" color and may have an underline. From ba705ce0362a2028c619caa670a74aee21d04403 Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Wed, 10 Apr 2024 12:09:50 -0500 Subject: [PATCH 10/10] CSS ordering --- packages/nimble-components/src/anchor/styles.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/nimble-components/src/anchor/styles.ts b/packages/nimble-components/src/anchor/styles.ts index fcb3959759..db4c6f3df0 100644 --- a/packages/nimble-components/src/anchor/styles.ts +++ b/packages/nimble-components/src/anchor/styles.ts @@ -25,10 +25,6 @@ export const styles = css` display: contents; } - [part='start'] { - display: none; - } - .control { color: ${linkFontColor}; text-decoration: underline; @@ -42,6 +38,10 @@ export const styles = css` color: ${linkProminentFontColor}; } + [part='start'] { + display: none; + } + .content { pointer-events: none; }