Link: Adding underline prop#16625
Conversation
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 16f45ce178b34cb87e9f8bcc8bfa0299c540c085 (build) |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 8572a0c:
|
ecraig12345
left a comment
There was a problem hiding this comment.
Couple minor suggestions but otherwise LGTM, thanks!
| className?: string; | ||
| isButton?: boolean; | ||
| isDisabled?: boolean; | ||
| isUnderlined?: boolean; |
There was a problem hiding this comment.
Wonder if this should be called just underline to match the prop name? But I could go either way on this since some of the other Link style props use the is convention.
There was a problem hiding this comment.
Yeah, went with the convention here but I can change that if wanted.
| a: { | ||
| component: MarkdownLink, | ||
| props: { className: 'ms-mdLink', styles: subComponentStyles.link }, | ||
| props: { className: 'ms-mdLink', styles: subComponentStyles.link, underline: false }, |
There was a problem hiding this comment.
I'm confused, how did this fix it? I would have expected it to make the links never have an underline?
There was a problem hiding this comment.
Hmm I don't think this is right, because it removes the underlines by default. I think links in markdown really ought to be underlined by default, for example these ones.
There was a problem hiding this comment.
To clarify, the reason I think markdown links ought to be underlined by default is because markdown is for body text, and links in body text should be underlined.
There was a problem hiding this comment.
Ok thinking about this more, the best solution I can see is different handling of related links all-up (pass an array not use markdown...there's no reason to define a bunch of identically formatted ULs in markdown).
So my suggestion for this PR would be to revert this line (so that all the links except related are correctly underlined), and I'll make a follow-up changing the related link formatting.
packages/react-docsite-components/src/components/Markdown/Markdown.tsx
Outdated
Show resolved
Hide resolved
|
🎉 Handy links: |
|
🎉 Handy links: |
#### Pull request checklist - [x] Addresses an existing issue: Fixes #16302 - [x] Include a change request file using `$ yarn change` #### Description of changes _Port of #16625 and #16721_ This PR adds an `underline` prop that provider underline styling to the `Link` component. This is done to add a styling difference in places where color is not enough contrast to differentiate the `Link` (i.e. cases where the `Link` is alongside other text).
Pull request checklist
$ yarn changeDescription of changes
This PR adds an
underlineprop that provider underline styling to theLinkcomponent. This is done to add a styling difference in places where color is not enough contrast to differentiate theLink(i.e. cases where theLinkis alongside other text).