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.

Sign up for GitHub

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

Jump to bottom

Fix link within heading font #7521

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix link within heading font #7521

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
605e3df
Fix link within heading font
rishy-2021 Aug 25, 2023
26397b0
change font of heading link
rishy-2021 Aug 26, 2023
754e98e
memoize textStyles
rishy-2021 Aug 27, 2023
150c0ef
fixes indentation in textStyles
rishy-2021 Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion app/components/post_list/post/body/message/message.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@
const animatedStyle = useShowMoreAnimatedStyle(height, maxHeight, open);
const style = getStyleSheet(theme);
const blockStyles = getMarkdownBlockStyles(theme);
const textStyles = getMarkdownTextStyles(theme);
const textStyles = useMemo(() => {
const _textStyles = getMarkdownTextStyles(theme);
return {..._textStyles, link:{..._textStyles.link, fontFamily:'Metropolis'}}

Check failure on line 65 in app/components/post_list/post/body/message/message.tsx

View workflow job for this annotation

GitHub Actions / test 

Missing space before value for key 'link'

Check failure on line 65 in app/components/post_list/post/body/message/message.tsx

View workflow job for this annotation

GitHub Actions / test 

Missing space before value for key 'fontFamily'

Check failure on line 65 in app/components/post_list/post/body/message/message.tsx

View workflow job for this annotation

GitHub Actions / test 

Missing semicolon
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

},[theme]);

Check failure on line 66 in app/components/post_list/post/body/message/message.tsx

View workflow job for this annotation

GitHub Actions / test 

A space is required after ','

const mentionKeys = useMemo(() => {
return currentUser?.mentionKeys;
Expand Down
Loading