Updates mfm-js and adds position, scale, fg and bg function to mfm render #385

Manually merged
Johann150 merged 5 commits from puniko/FoundKey:update-mfm-core into main 2023-05-27 10:14:40 +00:00
Contributor

Updates mfm-js to version 0.23.3 and adds the additional functions misskey has added a while back

update of mfm-js was nessecary to get it to parse negative numbered arguments like $[position.x=-1 test]

Updates mfm-js to version 0.23.3 and adds the additional functions misskey has added a while back update of mfm-js was nessecary to get it to parse negative numbered arguments like `$[position.x=-1 test]`
puniko added 4 commits 2023-05-21 16:45:51 +00:00
ci/woodpecker/pr/lint-client Pipeline was successful Details
ci/woodpecker/pr/lint-backend Pipeline was successful Details
ci/woodpecker/pr/build Pipeline was successful Details
ci/woodpecker/pr/lint-foundkey-js Pipeline was successful Details
ci/woodpecker/pr/lint-sw Pipeline was successful Details
ci/woodpecker/pr/test Pipeline was successful Details
5f9ab12596
update mfm-js lib
Author
Contributor

probably needs a clean-all to get it working on update btw

probably needs a clean-all to get it working on update btw
puniko added 1 commit 2023-05-21 17:16:55 +00:00
ci/woodpecker/pr/lint-foundkey-js Pipeline was successful Details
ci/woodpecker/pr/lint-client Pipeline was successful Details
ci/woodpecker/pr/build Pipeline was successful Details
ci/woodpecker/pr/lint-backend Pipeline was successful Details
ci/woodpecker/pr/lint-sw Pipeline was successful Details
ci/woodpecker/pr/test Pipeline was successful Details
5788c675c7
parseSimple in backend
First-time contributor

Would it be worth adding in these changes to the cheat sheet too?

https://akkoma.dev/FoundKeyGang/FoundKey/src/branch/main/packages/client/src/pages/mfm-cheat-sheet.vue

Would it be worth adding in these changes to the cheat sheet too? https://akkoma.dev/FoundKeyGang/FoundKey/src/branch/main/packages/client/src/pages/mfm-cheat-sheet.vue
Johann150 requested changes 2023-05-21 19:14:44 +00:00
@ -51,3 +54,3 @@
};
const genEl = (ast: mfm.MfmNode[]) => ast.map((token): VNode | VNode[] => {
const genEl = (ast: mfm.MfmNode[], scale: Number) => ast.map((token): VNode | VNode[] => {
Owner

I don't understand what this scale parameter is for, it doesn't seem to be read at any point and having to carry it through is kind of annoying.

I don't understand what this `scale` parameter is for, it doesn't seem to be read at any point and having to carry it through is kind of annoying.
Johann150 marked this conversation as resolved
@ -188,0 +197,4 @@
case 'scale': {
const x = Math.min(parseFloat(token.props.args.x ?? '1'), 5);
const y = Math.min(parseFloat(token.props.args.y ?? '1'), 5);
style = `transform: scale(${x}, ${y});`;
Owner

Not really happy with this because this allows to bypass the limitations that were created for x2, x3 and x4 to not allow someone else to insert MFM resulting in people having HUGE things on their timeline that they have to scroll by for hours.

My idea to fix it would be to use CSS and add it to these existing rules for x2, x3 and x4 functions: 7a94e9f2d5/packages/client/src/components/global/misskey-flavored-markdown.vue (L33-L57) (I'm not entirely sure if that works because those rules use font-size while you used transform: scale.)

Not really happy with this because this allows to bypass the limitations that were created for `x2`, `x3` and `x4` to not allow someone else to insert MFM resulting in people having HUGE things on their timeline that they have to scroll by for hours. My idea to fix it would be to use CSS and add it to these existing rules for `x2`, `x3` and `x4` functions: https://akkoma.dev/FoundKeyGang/FoundKey/src/commit/7a94e9f2d5804b6331272cc5d9db2ff12d8fde05/packages/client/src/components/global/misskey-flavored-markdown.vue#L33-L57 (I'm not entirely sure if that works because those rules use `font-size` while you used `transform: scale`.)
First-time contributor

Transform's don't affect the content height

If you don't want it to overflow into the replies, then you should add overflow: hidden or clip to the note (unless you already have)

Transform's don't affect the content height If you don't want it to overflow into the replies, then you should add overflow: hidden or clip to the note (unless you already have)
Owner

Thanks for pointing this out. I guess this is not an issue then.

Thanks for pointing this out. I guess this is not an issue then.
Johann150 marked this conversation as resolved
@ -188,0 +203,4 @@
}
case 'fg': {
let color = token.props.args.color;
if (!/^[0-9a-f]{3,6}$/i.test(color)) color = 'f00';
Owner

I think this should have an explicit check for undefined (if the argument is not specified at all) and true (if the argument is specified without value).

All values are coerced to strings, so omitting it or passing undefined causes test() to search for the string "undefined", which is rarely what you want.
-- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test

I also think the regex is incorrect since {3,6} means "3 to 6 repetitions" and not "3 or 6 repetitions". I.e. this would allow 4 and 5 character colour codes which I think is not understood by browsers.

Putting the default if it is an invalid colour seems a bit weird to me but I guess if that's what Misskey does we should probably do the same.

I think this should have an explicit check for `undefined` (if the argument is not specified at all) and `true` (if the argument is specified without value). > All values are coerced to strings, so omitting it or passing `undefined` causes `test()` to search for the string `"undefined"`, which is rarely what you want. -- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test I also think the regex is incorrect since `{3,6}` means "3 to 6 repetitions" and not "3 or 6 repetitions". I.e. this would allow 4 and 5 character colour codes which I think is not understood by browsers. Putting the default if it is an invalid colour seems a bit weird to me but I guess if that's what Misskey does we should probably do the same.
Owner

Recommended alternative regex: /^[0-9a-fA-F]{3}([0-9a-fA-F]{3})?$/ or so.

Recommended alternative regex: `/^[0-9a-fA-F]{3}([0-9a-fA-F]{3})?$/` or so.
Owner

or /^([0-9a-f]{3}){1,2}$/i stolen from this gist

or `/^([0-9a-f]{3}){1,2}$/i` stolen from [this gist](https://gist.github.com/olmokramer/82ccce673f86db7cda5e)
Johann150 marked this conversation as resolved
@ -188,0 +209,4 @@
}
case 'bg': {
let color = token.props.args.color;
if (!/^[0-9a-f]{3,6}$/i.test(color)) color = 'f00';
Owner

The comment for fg applies here as well of course.

The comment for `fg` applies here as well of course.
Johann150 marked this conversation as resolved
Johann150 added a new dependency 2023-05-27 09:53:38 +00:00
Johann150 manually merged commit 351a037959 into main 2023-05-27 10:14:40 +00:00
Sign in to join this conversation.
No reviewers
No Label
feature
fix
upkeep
No Milestone
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Blocks
Reference: FoundKeyGang/FoundKey#385
No description provided.