FoundKeyGang/FoundKey
FoundKeyGang/FoundKey
7
19
Fork 22
Code Issues 37 Pull requests 4 Releases Activity

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
Conversation 2 Commits 5 Files changed 6 +32 -21
2 changed files with 32 additions and 21 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit eb8463b292 - Show all commits

51
packages/client/src/components/mfm.ts
View file

@ -37,6 +37,10 @@ export default defineComponent({
type: Boolean,
default: true,
},
rootScale: {
type: Number,
default: 1
}
},
render() {
@ -50,7 +54,7 @@ export default defineComponent({
return t.match(/^[0-9.]+s$/) ? t : null;
};
Johann150 marked this conversation as resolved
Johann150 commented 2023-05-21 18:58:36 +00:00
Review
Copy link

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.
const genEl = (ast: mfm.MfmNode[]) => ast.map((token): VNode | VNode[] => {
const genEl = (ast: mfm.MfmNode[], scale: Number) => ast.map((token): VNode | VNode[] => {
switch (token.type) {
case 'text': {
const text = token.props.text.replace(/(\r\n|\n|\r)/g, '\n');
@ -69,17 +73,17 @@ export default defineComponent({
}
case 'bold': {
return h('b', genEl(token.children));
return h('b', genEl(token.children, scale));
}
case 'strike': {
return h('del', genEl(token.children));
return h('del', genEl(token.children, scale));
}
case 'italic': {
return h('i', {
style: 'font-style: oblique;',
}, genEl(token.children));
}, genEl(token.children, scale));
}
case 'fn': {
@ -139,18 +143,18 @@ export default defineComponent({
}
case 'x2': {
return h('span', {
class: 'mfm-x2',
}, genEl(token.children));
class: 'mfm-x2'
}, genEl(token.children, scale * 2));
}
case 'x3': {
return h('span', {
class: 'mfm-x3',
}, genEl(token.children));
class: 'mfm-x3'
}, genEl(token.children, scale * 3));
}
case 'x4': {
return h('span', {
class: 'mfm-x4',
}, genEl(token.children));
class: 'mfm-x4'
}, genEl(token.children, scale * 4));
}
case 'font': {
const family =
@ -167,7 +171,7 @@ export default defineComponent({
case 'blur': {
return h('span', {
class: '_mfm_blur_',
}, genEl(token.children));
}, genEl(token.children, scale));
}
case 'rainbow': {
const speed = validTime(token.props.args.speed) || '1s';
@ -176,9 +180,9 @@ export default defineComponent({
}
case 'sparkle': {
if (!this.$store.state.animatedMfm) {
return genEl(token.children);
return genEl(token.children, scale);
}
return h(MkSparkle, {}, genEl(token.children));
return h(MkSparkle, {}, genEl(token.children, scale));
}
case 'rotate': {
const degrees = (typeof token.props.args.deg === 'string' ? parseInt(token.props.args.deg) : null) || '90';
@ -191,26 +195,33 @@ export default defineComponent({
style = `transform: translateX(${x}em) translateY(${y}em);`;
break;
}
case 'scale': {
const x = Math.min(parseFloat(token.props.args.x ?? '1'), 5);
const y = Math.min(parseFloat(token.props.args.y ?? '1'), 5);
Johann150 marked this conversation as resolved
Johann150 commented 2023-05-21 19:04:34 +00:00
Review
Copy link

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:

Lines 33 to 57 in 7a94e9f
.mfm-x2 {
--mfm-zoom-size: 200%;
}
.mfm-x3 {
--mfm-zoom-size: 400%;
}
.mfm-x4 {
--mfm-zoom-size: 600%;
}
.mfm-x2, .mfm-x3, .mfm-x4 {
font-size: var(--mfm-zoom-size);
.mfm-x2, .mfm-x3, .mfm-x4 {
/* only half effective */
font-size: calc(var(--mfm-zoom-size) / 2 + 50%);
.mfm-x2, .mfm-x3, .mfm-x4 {
/* disabled */
font-size: 100%;
}
}
}

(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`.)
freeplay commented 2023-05-25 23:53:34 +00:00
Review
Copy link

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)
👀 1
Johann150 commented 2023-05-27 09:33:05 +00:00
Review
Copy link

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.
style = `transform: scale(${x}, ${y});`;
scale = scale * Math.max(x, y);
break;
}
}
if (style == null) {
Johann150 marked this conversation as resolved
Johann150 commented 2023-05-21 19:13:23 +00:00
Review
Copy link

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.
toast commented 2023-05-26 18:00:42 +00:00
Review
Copy link

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.
👍 1
Johann150 commented 2023-05-26 19:24:20 +00:00
Review
Copy link

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)
return h('span', {}, ['$[', token.props.name, ' ', ...genEl(token.children), ']']);
return h('span', {}, ['$[', token.props.name, ' ', ...genEl(token.children, scale), ']']);
} else {
return h('span', {
style: 'display: inline-block;' + style,
}, genEl(token.children));
}, genEl(token.children, scale));
}
Johann150 marked this conversation as resolved
Johann150 commented 2023-05-21 19:14:09 +00:00
Review
Copy link

The comment for fg applies here as well of course.

The comment for `fg` applies here as well of course.
}
case 'small': {
return h('small', {
class: '_mfm_small_'
}, genEl(token.children));
}, genEl(token.children, scale));
}
case 'center': {
return h('div', {
style: 'text-align:center;',
}, genEl(token.children));
}, genEl(token.children, scale));
}
case 'url': {
@ -226,7 +237,7 @@ export default defineComponent({
key: Math.random(),
url: token.props.url,
rel: 'nofollow noopener',
}, genEl(token.children));
}, genEl(token.children, scale));
}
case 'mention': {
@ -264,7 +275,7 @@ export default defineComponent({
case 'quote': {
return h(this.nowrap ? 'span' : 'div', {
class: 'quote',
}, genEl(token.children));
}, genEl(token.children, scale));
}
case 'emojiCode': {
@ -317,6 +328,6 @@ export default defineComponent({
}).flat();
// Parse ast to DOM
return h('span', genEl(ast));
return h('span', genEl(ast, this.rootScale ?? 1));
},
});

2
packages/client/src/scripts/mfm-tags.ts
View file

@ -1 +1 @@
export const MFM_TAGS = ['tada', 'jelly', 'twitch', 'shake', 'spin', 'jump', 'bounce', 'flip', 'x2', 'x3', 'x4', 'font', 'blur', 'rainbow', 'sparkle', 'rotate', 'position'];
export const MFM_TAGS = ['tada', 'jelly', 'twitch', 'shake', 'spin', 'jump', 'bounce', 'flip', 'x2', 'x3', 'x4', 'font', 'blur', 'rainbow', 'sparkle', 'rotate', 'position', 'scale'];