Minor UI performance improvements #89

Manually merged

Johann150 merged 1 commit from vib/FoundKey:perf/emoji-picker into main

2022-08-28 17:23:10 +00:00

vib commented

2022-08-27 11:15:46 +00:00

Contributor

Just merged a few commits from Misskey which contain minor UI performance improvements.
One of them improves the emoji picker's search speed, which should solve issue #72.

(this is the first time I PR something, hope I didn't do anyting wrong..^^;)

Just merged a few commits from Misskey which contain minor UI performance improvements. One of them improves the emoji picker's search speed, which should solve issue #72. (this is the first time I PR something, hope I didn't do anyting wrong..^^;)

Johann150 requested changes 2022-08-28 12:35:19 +00:00

Johann150 left a comment

Owner

Just for my (and others) understanding: The (S)CSS style changes can't really change the performance much. So what matters here must be the added v-onces which tell Vue to not re-render that tag/component ever.

But I'm not opposed to adding class names that make sense to things instead of having selectors based on the tag name so that part is probably nice to have too.

From my side there's only one thing to fix: Please translate the added comment in packages/client/src/components/emoji-picker.section.vue to English. We shouldn't add even more Japanese comments.

Just for my (and others) understanding: The (S)CSS style changes can't really change the performance much. So what matters here must be the added `v-once`s which tell Vue to not re-render that tag/component ever. But I'm not opposed to adding class names that make sense to things instead of having selectors based on the tag name so that part is probably nice to have too. From my side there's only one thing to fix: Please translate the added comment in `packages/client/src/components/emoji-picker.section.vue` to English. We shouldn't add even more Japanese comments.

toast commented

2022-08-28 12:43:44 +00:00

Owner

It is actually possible for the emoji part specifically to change the performance a lot though, since applying some styling to thousands of emojis one by one in a linear search would be a lot slower - so I do actually suggest trying to measure it.

The v-once is non-controversial though, since refreshes are already required for new emojis to show up. If it was this change alone I would just merge it ASAP.

Another optimization would be the selection code (looking at it, it's rather poor) - I'll try and tackle that today (whenever I have time).

The main issue with using classes this way is that they take up the global namespace (i.e if we want to have a global "body" class at some point, it will conflict). Scoped SCSS works the way it does as-is, so unless there's a huge measurable performance improvement, I'd rather not. It is actually possible for the emoji part specifically to change the performance a lot though, since applying some styling to thousands of emojis one by one in a linear search would be a lot slower - so I do actually suggest trying to measure it. The `v-once` is non-controversial though, since refreshes are already required for new emojis to show up. If it was this change alone I would just merge it ASAP. Another optimization would be the selection code (looking at it, it's rather poor) - I'll try and tackle that today (whenever I have time).

vib commented

2022-08-28 13:42:57 +00:00

Author

Contributor

yeah, sorry, I kinda took those commits for granted.
So, dump everything except v-once and the emoji class (if the emoji class does prove to be any worth after testing)?

yeah, sorry, I kinda took those commits for granted. So, dump everything except v-once and the emoji class (if the emoji class does prove to be any worth after testing)?

vib force-pushed perf/emoji-picker from c3ebecfc0c to 62ff65fbf4

2022-08-28 16:23:05 +00:00

Compare

vib commented

2022-08-28 16:27:32 +00:00

Author

Contributor

well, the using the emoji class probably doesn't change much. I just added the v-once things now.

vib requested review from Johann150 2022-08-28 16:28:34 +00:00

vib commented

2022-08-28 17:01:27 +00:00

Author

Contributor

Also I hope me doing a git reset and force push in this case isn't.. wrong. I thought if this is all I am going to add there is no reason to have syuilo's commits in between.
Still learning the ettiquete of dealing with a project..

Also I hope me doing a git reset and force push in this case isn't.. wrong. I thought if this is all I am going to add there is no reason to have syuilo's commits in between. Still learning the ettiquete of dealing with a project..