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

[BUG] TW UI uses "tc-reveal" class so exessively, that it has no value for styling anymore #8709

Open
pmario opened this issue Oct 29, 2024 · 9 comments

Comments

@pmario
Copy link
Member

pmario commented Oct 29, 2024

Describe the bug

Talking about "cruft".

The core UI uses tc-reveal so excessively, that it has absolutely no value for styling anymore. The only thing it does is wasting CPU cycles needed for parsing.

Expected behavior

We should remove it.

tc-reveal is not used for styling in any core theme CSS tiddlers. -- (I think for a reason)

To Reproduce

See screenshot below

title: show-tc-reveal
tags: $:/tags/Stylesheet

.tc-reveal {
  outline: 2px solid red;
}

Screenshots

image

TiddlyWiki Configuration

v5.3.5

Additional context

No response

@CodaCodr
Copy link
Contributor

Blindly removing it will "break" anyone's wiki that is using tc-reveal.

.something .something-else .tc-reveal .my-thing { ...

@pmario
Copy link
Member Author

pmario commented Oct 30, 2024

We did already remove a lot of them when we changed from reveal-widget to <%if%> -- We did not get a single issue yet.

@CodaCodr
Copy link
Contributor

Absence of evidence is not evidence of absence.

If you (we) remove it, it MUST be presented as a potentially breaking change.

@CodaCodr
Copy link
Contributor

I did a search of a couple of my wikis. Tobi Beer's appear plugin uses it. My code uses it in two places. I can fix those easily enough -- and thanks to bundler, I can propagate it too ;)

I fear there are more plugins out there that might use it, and other folks that may have long-forgotten CSS relying on it.

Either way, "it has no value for styling anymore" is not a true statement.

@Jermolene
Copy link
Member

Hi @pmario sadly I think this is indeed something that would be problematic to change at this point. Perhaps the best thing we could do is to hasten the deprecation of the reveal widget. I think the current obstacle is that it is the only way to make an animated appearance/disappearance.

@ericshulman
Copy link
Member

I also have used .tc-reveal in https://tiddlytools.com/#TiddlyTools%2FCatalog, which contains this CSS:

.tt-catalog {
	& .tc-tab-content .tc-reveal>p	{ margin:0; }

which eliminates unwanted top/bottom margins for the first <p> element within that specific .tc-reveal

For this particular use-case, I could replace .tc-reveal with >div so that it no longer depends upon the class name, like this:

.tt-catalog {
	& .tc-tab-content >div>p		{ margin:0; }

However, this example illustrates that completely removing .tc-reveal from the $reveal widget output can produce visible backward-compatibility issues for some people.

As far as TWCore performance is concerned, I suggest just continuing to replace the use of $reveal widgets with <%if%> whenever possible, but leave the use of .tc-reveal within the $reveal widget implementation for those who are currently using it in their plugins or wiki content.

@Leilei332
Copy link
Contributor

Actually I wonder the difference between the reveal widget and conditional shortcut (which generates a list widget), and which one has a better performance.

@Jermolene
Copy link
Member

Actually I wonder the difference between the reveal widget and conditional shortcut (which generates a list widget), and which one has a better performance.

I think here we should be more concerned with making the core simpler for people to understand than marginal performance differences. The reveal widget should really be called the "animate" widget because that is the only remaining use case for it. For all other cases the conditional shortcut syntax is more readable and flexible.

@ericshulman
Copy link
Member

ericshulman commented Oct 31, 2024

the reveal widget... is the only way to make an animated appearance/disappearance.

Even without animation, the "retain" feature of the reveal widget can be used as an optional parameter in the <<tabs>> macro. This can be important for efficient tab switching, particularly if the tab content has a high initial rendering overhead, but a low occurence of refresh triggers.

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

No branches or pull requests

5 participants