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

Sidebar-resizer: change $:/themes/... tiddlers #8663

Draft
wants to merge 195 commits into
base: master
Choose a base branch
from

Conversation

BurningTreeC
Copy link
Contributor

@BurningTreeC BurningTreeC commented Oct 4, 2024

This PR uses the same mechanism as #8644 but changes the $:/themes/tiddlywiki/vanilla/metrics/... tiddlers directly.
Note that in fixed-fluid mode the story-river actually changes its width but the tiddlerwidth remains fixed width as there was no discussion yet how we handle this. I just made the tiddlers adapt to the story-river if the story-river-width goes below the tiddlerwidth.

There are some design questions:

  • should it be possible to make the story-river overlap the sidebar / the slider?
  • should the tiddlerwidth in fixed-fluid mode be updated? (keeping the gap between sidebar and story-river)

I haven't yet added the ControlPanel configurations used for storyminwidth and sidebarminwidth.
This PR is just for completeness, I will update it so that we can compare with other possible solutions.


In the meantime this PR has been updated quite a bit, I've put a current build here:

https://sidebar-resizer-config.tiddlyhost.com/

Copy link

stackblitz bot commented Oct 4, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

github-actions bot commented Oct 4, 2024

Confirmed: BurningTreeC has already signed the Contributor License Agreement (see contributing.md)

@BurningTreeC
Copy link
Contributor Author

@Jermolene @saqimtiaz @pmario

sorry for tagging you 😸
But if you have time, could you test?
Especially fixed-fluid mode needs testing.
This here now works with pixel and percentage values.
It updates the $:/themes/... tiddlers directly.
In fixed-fluid mode it updates storyright and storyleft.
There's also the option to slide using ctrl in fixed-fluid mode. ctrl makes it possible to move the sidebar further to the left if there's a gap between story and sidebar. To test that, maybe set the storyright to 75% and the storywidth to 45% and the tiddlerwidth to 100% ... it becomes easier to understand like that.

The calculations are now done in wikitext in the procedures.
One thing I had to limit to not become less than 0 which is the storyleft.

@BurningTreeC
Copy link
Contributor Author

This works now with all absolute css metrics:

Bildschirmfoto vom 2024-10-06 10-41-34

Including percentage, which is easy to calculate.
If a metric was for instance "pt" or "%" before starting to resize, it will be updated as "pt" / "%" and so on.

The internal calculations are all done in pixel, so this first converts everything to pixel, then calculates, then converts back to the corresponding value.

I don't know if this can also support other relative css metrics...

@BurningTreeC
Copy link
Contributor Author

As I said above, in fixed-fluid mode I'm not settled about how the gap between story-river and sidebar should be handled.
This implementation however can be modified easily in that regards

@pmario
Copy link
Member

pmario commented Oct 6, 2024

@BurningTreeC -- I did checkout this branch and did run it, but I can not find any resizer. I think I'm missing something


\procedure get_theme-tweaks_lingo() <$transclude $variable="lingo" title={{{ [[Metrics/]addsuffix<metricName>] }}}/>

\procedure get_theme-tweaks_lingo-hint() <$transclude $variable="lingo" title={{{ [[Metrics/]addsuffix<metricName>addsuffix[/Hint]] }}}/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not use underscores in the core at the moment and you should not start with it. It will cause consistency problems. So get-theme-... should be fine

If you think it is too similar to the function names, getThemeTweaksLingo() may be an option too

@@ -2,6 +2,12 @@ title: $:/themes/tiddlywiki/vanilla/themetweaks
tags: $:/tags/ControlPanel/Appearance
caption: {{$:/language/ThemeTweaks/ThemeTweaks}}

\function get.theme-tweaks.metric() [get.theme<metric>]

\procedure get_theme-tweaks_lingo() <$transclude $variable="lingo" title={{{ [[Metrics/]addsuffix<metricName>] }}}/>
Copy link
Member

@pmario pmario Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try substitution instead of a filtered transclusion <$transclude $variable="lingo" title=`Metrics/$(metricName)$`/> It is simpler

</$let>
\end

\procedure two-cell-slider(width:"100%",minHeight:"10px",templateLeft:"",templateRight:"",mode:"block",sliderWidth:"12px",padding:"12px",sliderCondition:"yes",leftMinWidth:"100px",rightMinWidth:"100px")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

procedure parameters allow newlines. So it should be possible to make this line more readable

tv-set-storywidth-storyright="no"
tv-set-sidebarwidth="yes"
tv-set-centralised="no"
storyLeftTiddler={{{ [<storyLeftTiddler>!is[blank]] :else[<qualify>addsuffix[-]addsuffix<currentTiddler>sha256[]addprefix[$:/state/resizer/storyleft-]] }}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the :else path the first addsuffix[-] after <qualify> is redundant, since it is turned into a number with sha256[]

So removing the first addsuffix[-] should make the string a little bit shorter

\function convert.to.in(value) [<value>divide[96]]
\function convert.to.pc(value) [convert.to.in<value>multiply[6]]
\function convert.to.pt(value) [convert.to.in<value>multiply[72]]
\function convert.to.em(value) [[storyTiddler]is[variable]then<value>divide{$:/themes/tiddlywiki/vanilla/metrics/bodyfontsize}] [[storyTiddler]!is[variable]then<value>divide{$:/themes/tiddlywiki/vanilla/metrics/fontsize}]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure, but I think if storyTiddler in the function above is a variable the then will create a value. But there is a second element without an :else -- So I am not sure if this filter will work. Did you test it?

@@ -26,17 +28,34 @@ Metrics/FontSize: Font size
Metrics/LineHeight: Line height
Metrics/BodyFontSize: Font size for tiddler body
Metrics/BodyLineHeight: Line height for tiddler body
Metrics/PreviewSliderWidth: Preview slider width
Metrics/PreviewSliderWidth/Hint: the width of the slider between editor and editor preview
Copy link
Member

@pmario pmario Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those hints within a sentence? Otherwise they should start with capital letters

list-after: $:/core/ui/PageTemplate/story
code-body: yes

\import [function[get.base.functions.theme],<get.current.theme>first[]is[tiddler]] :else[function[get.base.functions.theme],<get.current.theme>first[]is[shadow]] :else[[$:/themes/tiddlywiki/vanilla/functions]] [[$:/core/procedures/sidebar-resizer]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add an empty line after \import for better readability

\function drag.direction.reverse() [<get.theme.option sidebarposition>match[left]then[yes]] :else[[no]]
\whitespace trim

<%if [<get.theme.explicit.option sidebarresizer>match[show]] %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<%if %> should not need an empty line, if the block is indented.

storyPaddingRightTiddler=<<get.story-padding-right.tiddler>>
>

<$transclude $variable="sidebar-resizer"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the leading and trailing new-lines should be removed. I am not sure if <$transclude will need a $mode="block" or not. But there should be no empty lines here

@pmario
Copy link
Member

pmario commented Nov 13, 2024

I think the editor preview state is broken. The preview icon does not change if the preview is active or not

image

@BurningTreeC
Copy link
Contributor Author

I think the editor preview state is broken. The preview icon does not change if the preview is active or not

image

Oh, I'll fix this

@BurningTreeC
Copy link
Contributor Author

made also a quick change that broke the styling of the slider - sorry

@pmario
Copy link
Member

pmario commented Nov 13, 2024

made also a quick change that broke the styling of the slider - sorry

No need to be sorry. It's still WIP. At the moment I do have a look, if we can make some functions simpler.

BTW -- you can add some <!-- comments --> outside of procedures or functions with no performance hit. If "something special" is going on they should help others and probably yourself too.

I use comments whenever my own functions or procs, "trick" me and I change something. Just to find out, that I introduced an error.

just some thoughts

@BurningTreeC
Copy link
Contributor Author

made also a quick change that broke the styling of the slider - sorry

No need to be sorry. It's still WIP. At the moment I do have a look, if we can make some functions simpler.

BTW -- you can add some <!-- comments --> outside of procedures or functions with no performance hit. If "something special" is going on they should help others and probably yourself too.

I use comments whenever my own functions or procs, "trick" me and I change something. Just to find out, that I introduced an error.

just some thoughts

I don't even know what I'm doing 😉 so how should I comment it?
No, just kidding ... but I'm having a hard time commenting what I'm doing.
Where I would need comments it's not possible ... and where I could comment I haven't yet found the words

@@ -1,18 +1,18 @@
title: $:/core/ui/EditTemplate/body/default

\function get.edit-preview-state()
\function edit-preview-state()
Copy link
Member

@pmario pmario Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

functions need a dot . in the function name. Otherwise thy can only be used with [function[get-preview-state]] -- For the core we need the possibility to use it with [tf.get-preview-state<something>] too.

That's why I did start with tf. prefix eg: tf.function-name-with-hyphens for TW global functions. If you search the source code for tf., you will see some of them in the TW core code already.

At the moment they are only used for $:/tags/Global functions.

Copy link
Member

@pmario pmario Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the draft pr branch https://github.com/pmario/TiddlyWiki5/blob/toc-v5.3.x-rewrite/core/wiki/macros/toc.tid you can see it used extensively. Otherwise the code would be unreadable. There is some info about the naming schema in code.

@linonetwo
Copy link
Contributor

add some <!-- comments -->

Try use GPT o1 for this. BTW, I'm trying to use LLM to add comment to all TW core wikitext, if you aleady add some, LLM will thank you for making it easier :P

@BurningTreeC
Copy link
Contributor Author

Hi @Jermolene @saqimtiaz @pmario @linonetwo

I've now done some experiments using pointer capture (not in this PR) and couldn't achieve what we achieve here.
Especially the behavior for pointerleave / pointerout / pointercancel - I wouldn't know which tricks to use to achieve them.

Anyway, now this PR fixed an important problem which is the editor and preview redrawing when resizing. Fixed 🥳 !

I had to use some JS for factory.js so that - if the class of the editor changes, it just assigns the new classes to the DomNode.

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.

7 participants