Fix link within heading font #7521
Conversation
mm-cloud-bot
added
kind/bug
|
Hello @rishy-2021, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. |
app/utils/markdown/index.ts
Outdated
| @@ -45,7 +45,7 @@ export const getMarkdownTextStyles = makeStyleSheetFromTheme((theme: Theme) => { | |||
| }, | |||
| link: { | |||
| color: theme.linkColor, | |||
| fontFamily: 'OpenSans', | |||
| fontFamily: 'Metropolis', | |||
There was a problem hiding this comment.
this is not the proper fix, as a link in normal paragraph will now use Metropolis instead of openSans
There was a problem hiding this comment.
Sorry, i misunderstood the code , now only the heading link font is Metropolis
enahum
added
2: Dev Review
| @@ -94,7 +94,7 @@ const Message = ({currentUser, highlight, isEdited, isPendingOrFailed, isReplyPo | |||
| layoutWidth={layoutWidth} | |||
| location={location} | |||
| postId={post.id} | |||
| textStyles={textStyles} | |||
| textStyles={{...textStyles, link:{...textStyles.link, fontFamily:'Metropolis'}}} | |||
There was a problem hiding this comment.
If this is done you should Memoize it.
There was a problem hiding this comment.
@enahum is there anything left for me to change
| const textStyles = getMarkdownTextStyles(theme); | ||
| const textStyles = useMemo(() => { | ||
| const _textStyles = getMarkdownTextStyles(theme); | ||
| return {..._textStyles, link:{..._textStyles.link, fontFamily:'Metropolis'}} |
There was a problem hiding this comment.
Isn't this making all links (including those in the body) use Metropolis?
Also, I just saw the problem may be a bit more deep, since I can also reproduce the issue with something like ## heading **heading**. Also, this one would solve it only for the post, but anywhere else markdown may be used (like post attachments) will be broken also.
I guess the issue is in part with computeTextStyle in app/utils/markdown/index.ts and in part with getMarkdownTextStyles in the same file.
Some juggling may be needed, but I feel in most of the places the fontFamily should not be there, and be inherited from the parent, but 0/5 on the details.
There was a problem hiding this comment.
Oh very good points @larkox I do agree with you.. maybe we just need to add new styles for markdown as we have headingXText we could do the same for headingXLink where X is the number of the heading
There was a problem hiding this comment.
confirming that this indeed does make all link text use Metropolis. Let me know when this is updated and I can review again @rishy-2021
There was a problem hiding this comment.
The problem of adding a headingLink is that then you have to add one for each variation (take in mind that my example was no longer a heading link, but a bolded heading.
This should be solved in a more holistic way, hopefully trusting inheritance of styles.
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
@rishy-2021 Are you still working on this? |
Summary
change link font to Metropolis from OpenSans
Ticket Link
https://mattermost.atlassian.net/browse/MM-53978
Device Information
This PR was tested on: Realme GT2, realme UI 4.0 | Android 13
Release Note
NA