Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix evaluation of post content by m.trust() #24

Merged
merged 2 commits into from
Jan 22, 2021
Merged

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Jan 13, 2021

No description provided.

Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

Looking good and tested locally.

Should we add a comment maybe? Like

// Wrapping in <div> because ItemList entries need to be vnodes

Something else I thought of is that we can also use a text vnode. Unfortunately Mithril has no helper method to generate them, so that instantly makes the code less readable https://mithril.js.org/vnodes.html#vnode-types

This is how it would work if we wanted to use a text vnode. I tested it locally:

items.add('excerpt', {tag: "#", children: excerpt}, -100);

EDIT: also probably not an issue, but was this branched out of an older version? The fix from 55cc45c isn't present when I checkout this PR. Should fix itself by merging anyway.

@SychO9
Copy link
Member Author

SychO9 commented Jan 15, 2021

I don't have a strong opinion on using a text vnode or wrapping in div 🤷🏼‍♂️ as long as it works. I haven't made that change but we can do it if anyone wants to ofc.

EDIT: also probably not an issue, but was this branched out of an older version? The fix from 55cc45c isn't present when I checkout this PR. Should fix itself by merging anyway.

Ah yes, I seem to have forgotten to update my local master branch. Won't be a problem for git though.

Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

Tested locally again, all good for me 👍

Copy link
Member

@imorland imorland left a comment

Choose a reason for hiding this comment

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

tested locally. confirmed to resolve the issue

@askvortsov1 askvortsov1 merged commit 7ebd304 into master Jan 22, 2021
@luceos
Copy link
Member

luceos commented Mar 15, 2021

@SychO9 please provide a reason for a PR when creating one, that makes writing changelogs a ton easier ;)

@luceos luceos deleted the sm/dont-use-mtrust branch March 15, 2021 11:25
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants