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.

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

Single style tags - put stylesheets into single <style> tags #8118

Closed
wants to merge 25 commits into from

Conversation

BurningTreeC
Copy link
Contributor

This PR is an alternative to #8106
I open this as a Draft PR for Discussion.
This PR still uses the <$transclude mode="block"> widget since I couldn't figure out a fast way for the view widget to detect if its text changes. But here the transclude widget can simply be replaced.

The advantage of this PR is, that the <style> tags are not necessarily needed. If they are omitted, then the styles are put in one single style tag like it does currently.

The heavy lifting is done within the styleRefresh and the times will probably go up. Needs testing

Copy link

vercel bot commented Mar 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
tiddlywiki5 ✅ Ready (Inspect) Visit Preview Mar 31, 2024 8:39pm

@BurningTreeC
Copy link
Contributor Author

Oh I forgot to mention that here I've also added the modern-normalize 2.0.0 instead of the 1.0.0

@BurningTreeC
Copy link
Contributor Author

If I download an empty wiki from https://tiddlywiki.com/prerelease and an empty wiki from the Preview of this PR and I enable Performance Instrumentation on both, I get comparable results

@Jermolene
Copy link
Member

Thanks @BurningTreeC

Oh I forgot to mention that here I've also added the modern-normalize 2.0.0 instead of the 1.0.0

That should be a separate PR, please.

@BurningTreeC
Copy link
Contributor Author

Hi @Jermolene, I've reverted adding modern-normalize 2.0.0

Another question: What was your idea for the view widget to check if it needs to refresh?

@BurningTreeC BurningTreeC marked this pull request as ready for review March 29, 2024 15:46
@BurningTreeC
Copy link
Contributor Author

Hello @Jermolene
I added some logic to the view widget but I'm not happy with it because it makes the styleRefresh times worse.
It basically adds an option (attribute) update to the view widget that, if set to yes, creates a fake transclude widget (which is too heavy) and a fake domNode. In the refresh method then it refreshes the children of this fake widget and if the textContent of the fake domNode is different than this.text it updates the view widget's textContent and returns true.

I found that I need a fake widget and a fake domNode for this - do you have a better idea?
If I simply get the new text through getValueAsPlainWikified(this.viewMode) for example and check if it's different than this.text the styleRefresh times go up by 10 times. From 5ms to 50ms.
That's why I need a fake widget but using the transclude widget for this purpose is really too heavy. Is there another way?

Thank you,
Simon

@BurningTreeC
Copy link
Contributor Author

I temporarily enabled performance instrumentation for this PR, I will disable it at the end

core/modules/startup/render.js Outdated Show resolved Hide resolved
core/modules/startup/render.js Outdated Show resolved Hide resolved
core/modules/startup/render.js Outdated Show resolved Hide resolved
core/modules/startup/render.js Outdated Show resolved Hide resolved
core/modules/startup/windows.js Outdated Show resolved Hide resolved
core/modules/startup/windows.js Outdated Show resolved Hide resolved
@BurningTreeC
Copy link
Contributor Author

This PR is now ready for review. It doesn't incorporate changes to the view widget since therefor I added PR #8125
It uses the view widget attributes introduced in #8125 - that's why this should wait until #8125 is ready

This PR alone - without the changes in #8125 - will NOT update styles if anything changes

@BurningTreeC BurningTreeC marked this pull request as ready for review March 31, 2024 06:02
@Jermolene
Copy link
Member

Thank you @BurningTreeC, I appreciate your time on this.

I am afraid there is a bad smell from the fact that render.js has to track the style widgets and style elements between refreshes. That's really what I meant about duplicating the logic of the list widget; the hallmark of the list widget is that it keeps the previous list contents so that it can optimise changes. We should be able to leverage those existing optimisations.

Further I strongly think changes like this will need some tests to underpin them. We need to verify that the refresh mechanism works as expected, with no unexpected refreshes. There are some existing tests that show how this can be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants