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.

Sign up for GitHub

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

Jump to bottom

Fix list widget when widget body used as template while $list-empty specified #7810

Closed
wants to merge 3 commits into from
Closed

Fix list widget when widget body used as template while $list-empty specified #7810

wants to merge 3 commits into from

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Oct 24, 2023

If <$list-empty> is specified but <$list-template> is not, and the contents of the list widget are used as the template instead, then the result should be the same as if the emptyMessage attribute had been used.

Fixes #7804.

@rmunn
Fix list widget when only list-empty specified
2ff73ab 
If `<$list-empty>` is specified but `<$list-template>` is not, and the
contents of the list widget are used as the template instead, then the
result should be the same as if the emptyMessage attribute had been used.
@vercel
Copy link

vercel bot commented Oct 24, 2023 •
edited

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

Name Status Preview Updated (UTC)
tiddlywiki5 ✅ Ready (Inspect) Visit Preview Nov 1, 2023 8:45am

@rmunn rmunn mentioned this pull request Oct 24, 2023
Open
@pmario
Copy link
Contributor

pmario commented Oct 24, 2023

I think there is a problem with P-tag creation. IMO the code contains way too much handling for P-tag edge-cases.

All the special handling in this PR is fighting symptoms and does not fix the underlaying problem the TW syntax has with redundant P-tag creation.

@rmunn rmunn mentioned this pull request Oct 25, 2023
Merged
@rmunn
Copy link
Contributor Author

rmunn commented Oct 25, 2023

@Jermolene - I would appreciate a review of this PR, because it's blocking #7694 (comment). That is, I could work further on #7694, adding a <$list-join> pseudo-widget (as you suggested). But the code I would have to write to add it would be so much simpler after this PR is merged that I'd prefer to wait; if I do the work now against current master I'll end up dealing with merge conflicts later. So if you need me to change anything about this bugfix PR, please let me know and I'll work on it right away. Once this is merged then I'll be able to finish #7694 and mark it ready for review as well.

@rmunn
Copy link
Contributor Author

rmunn commented Oct 25, 2023

@pmario - You're right about the <p> tag being a problem. However, I wasn't about to make large structural changes to the TW parser in a PR that fixes a single corner-case bug. The <p> tag stuff you mentioned is the result of #7710 (comment), where if the list widget is in block mode then it wasn't picking up on the $list-template and $list-empty children (because, indeed, of that extra <p> tag). So my PR has to deal with that as well.

So yes, TW's handling of paragraph tags could use some looking at. I don't have time to do it at the moment, though.

@pmario
Copy link
Contributor

pmario commented Oct 25, 2023

So yes, TW's handling of paragraph tags could use some looking at. I don't have time to do it at the moment, though.

I think it's about 5 lines in 1 file to fix the problem core/modules/parsers/wikiparser/wikiparser.js.

It fixes a lot of problems with the TW DOM structure, but it also makes some problems visible that have been covered by erroneous P-tags. The linked PR contains 112 test tiddlers where we need to have a closer look.

This was referenced Oct 26, 2023
Closed
Merged
@rmunn
Improve performance of list template discovery
69b7192 
Since parse tree nodes never change after widget creation (whereas
attribute values *can* change), we can safely search for the explicit
list templtaes only once, at widget creation time. This saves time as
the search doesn't have to be done on each re-render, and also allows us
to safely do a clone-and-mutate step to extract the list widget's body
(if any) without any `$list-empty` or other items. That, in turn, allows
using the list widget's body as the template even if `$list-empty` is
specified inside the widget body.
@rmunn
Copy link
Contributor Author

rmunn commented Nov 3, 2023

Now that #7827 is working, closing this one in favor of #7827 which @Jermolene prefers. Will reopen if @Jermolene asks me to, of course.

@rmunn rmunn closed this Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] List template syntax inconsistent
2 participants
@rmunn @pmario