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

fix!: rework the clicks system #1279

Merged
merged 34 commits into from Feb 13, 2024
Merged

fix!: rework the clicks system #1279

merged 34 commits into from Feb 13, 2024

Conversation

KermanX
Copy link
Member

@KermanX KermanX commented Feb 8, 2024

Overview

This PR refactors the clicks system.

Affected:

  • v-click, v-after, v-click-hide directives
  • VClicks, CodeBlockWrapper, KaTexBlockWrapper, SlidevVideo components
  • new VClickGap component

Fixes:

Changes

Design

I personally think that the clicks system is a little ambiguous.

This PR considers the clicks to be whether relative or absolute.

<div v-click>        relative   show >= 1 </div>
<div v-click="'+1'"> relative   show >= 2 </div>
<div v-click="4">    absolute   show >= 4 </div>
<div v-click-hide>   relative   hide >= 3 </div>

'''js {1|2}{finally:'all',at:'+1'【default】}
1                          highlight <  4 || > 4
2                          highlight == 4 || > 4
3                          highlight         > 4
'''

'''js {hide|2|3}{at:2【absolute】}
1                          hide < 3
2                          hide < 3, highlight == 3
3                          hide < 3, highlight >= 4
'''

<div v-after>        relative   show >= 4  </div>
<div v-click="10">   absolute   show >= 10 </div>
<div v-click="'+2'"> relative   show >= 6  </div>

"relative" elements vs. previous auto-calculate clicks

Similar to previous auto-calculate clicks, "relative" elements' click num are calculated depending on the total click num of "relative" elements before this element.

Previously, all v-click elements, including those with an absolute click num, will add the auto-calculate clicks by one:

<div v-click="100"> I am absolute! </div>
<div v-click> The auto-calculate click num is 2, instead of 1 </div>

In this PR, only "relative" elements get auto-calculate clicks and add the "relative"s' current total click num by its "size" (for v-click it's 1, for v-click="'+3'" it's 3, for v-after it's 0). The example is above.

User-facing

  • VClicks.props.every now accepts string.

  • Code block/latex block line highlight range now supports 'none' and 'hide'.
    i.e. Accepts ranges like 1-3,5, or 'none', or 'hide'

    • 'none': No lines are highlighted
    • 'hide': The code block is hidden
  • Code block/latex block line highlight adds a finally prop.
    Default: 'last', the last highlight range. If no highlight animation, all lines are highlighted.
    This can also be ranges like 1-3,5, 'none', or 'hide'.
    The difference between this prop and the last one in the ranges: See fix!: rework the clicks system #1279 (comment).

  • The relative value of at prop like "+1" "-1" now affects the position of following "relative" elements.
    See fix!: rework the clicks system #1279 (comment) and fix!: rework the clicks system #1279 (comment)

    • v-click === v-click="'+1'"
    • v-after === v-click="'+0'"
  • Add v-after.fade and v-click-hide.fade for consistency with v-click.

  • Deprecate v-click.hide (it works the same as v-click-hide, and there hasn't been a v-after.hide)
    v-click.hide and v-after.hide are both available now.

  • Remove clicks in page frontmatter, because it is useless now restored.

Internal

Todos

  • Documentation. Help wanted: Since I am not a native English speaker, it is difficult for me to describe the changes introduced by this PR in the most precise and concise terms.
  • add v-after-hide or fix v-after to support this?
<div v-click-hide> Hide since 1 </div>
<div v-after>      Hide since 1 </div>

@antfu
Copy link
Member

antfu commented Feb 8, 2024

Hey, thanks a lot for the initiative! Indeed I think the clicks system needed some improvements.

Some feedback:

flow value

I am not very sure what flow means exactly - but it seems to be pretty similar to the previous auto-calculate clicks. If it's the default, I wonder what's the reason/need to introduce this value?

code block/latex block line highlight range now supports 'none' and 'hide'

I like it!

code block/latex block line highlight adds a finally prop

Is this necessary? People could do that with {1|2|*} to have the finally: all behavior. I would avoid introducing too many concepts especially if the current one works well

the relative value of at prop like "+1" "-1"is removed

I am not sure if it's internal. IIRC, it was a PR to add that support. If there is no strong technical reason, I would keep it to avoid making breaking changes

deprecate v-click.hide (it works the same as v-click-hide, and there hasn't been a v-after.hide)

Why not add .hide to v-after. Again I'd like to avoid breaking existing slides

remove clicks in page frontmatter, because it is useless now

This is definitely a no to me. I use it a lot in my slides. No matter how perfect the clicks inference we might have, we still need to have an escape hatch for users to manually specify the clicks they want. In addition, it's possible to have dynamic conditions like:

<div :class='$clicks > 5 ? 'some-class' : 'another-class'" />

@KermanX
Copy link
Member Author

KermanX commented Feb 8, 2024

@antfu Thank you for your support and affirmation.

it seems to be pretty similar to the previous auto-calculate clicks

It is. The previous auto-calculate clicks also consider the elements with specified "at", but this PR doesn't.

About the meaning of "flow", I am not very sure whether "flow" is the best name. I will try to explain it more clearly later.

code block/latex block line highlight adds a finally prop

Is this necessary? People could do that with {1|2|*} to have the finally: all behavior. I would avoid introducing too many concepts especially if the current one works well

Yes, it is. because {1|2|*} takes 2 clicks if it is in the "flow", but {1|2} only takes one. (p.s. the "a" in {a|b|c} means "before clicks into the code block" both in the past and in this PR).

I am not sure if it's internal. IIRC, it was a PR to add that support. If there is no strong technical reason, I would keep it to avoid making breaking changes

I considered it again and I now think this is meaningful. I will implement this maybe tomorrow (UTC+8).

Why not add .hide to v-after. Again I'd like to avoid breaking existing slides.

Nice idea

remove clicks in page frontmatter, because it is useless now

This is definitely a no to me. I use it a lot in my slides. No matter how perfect the clicks inference we might have, we still need to have an escape hatch for users to manually specify the clicks they want. In addition, it's possible to have dynamic conditions like:

I forgot that it is possible to use click system programatically😢. I will revert the removement.


By the way, could you have a look at vuejs/core#10295? This is needed in this PR to work with nested v-clicks properly.

@KermanX
Copy link
Member Author

KermanX commented Feb 8, 2024

the relative value of at prop like "+1" "-1"is removed

I am not sure if it's internal. IIRC, it was a PR to add that support. If there is no strong technical reason, I would keep it to avoid making breaking changes

I am wondering what's the actual usage of this relative value besides the internal usage.

Previously, v-click="'+2'" works like transform: translateX(2) instead of margin-left: 2 in CSS. It is still considered as "one click" in the "auto-calculate clicks":

<div v-click>        show >= 1 </div>
<div v-click="'+2'"> show >= 4 </div>
<div v-click>        show >= 3 </div>

I think this is kind of useless, because when the user wants to do this, he may write like this, which is much more direct:

<div v-click>        show >= 1 </div>
<div v-click="4">    show >= 4 </div>
<div v-click>        show >= 3 </div>

In my opinion, it is better to reserve this syntax but give it a similar meaning: like margin-left in CSS:

<div v-click>        show >= 1 </div>
<div v-click="'+2'"> show >= 3 </div> !!!!!
<div v-click>        show >= 4 </div> !!!!!

In this way, v-click="'flow'" is the same as v-click="'+1'"

@antfu
Copy link
Member

antfu commented Feb 9, 2024

v-click="'+2'" works like transform: translateX(2) instead of margin-left: 2 in CSS.

I don't understand what do you mean here.

<div v-click="4">show >= 4 </div>

Well, ofc. But absolute values are less flexible, especially if you have a lot of click elements in a single slide. Imagine you want to insert a new v-click before this, you have to manually bump 1 for every absolute clicks after that. In that case relative values could save a lot.

<div v-click>        show >= 1 </div>
<div v-click="'+2'"> show >= 3 </div> !!!!!
<div v-click>        show >= 4 </div> !!!!!

I like that, and yes this is probably more intuitive. We could do that.

v-click="'flow'" is the same as v-click="'+1'"

Sounds good, then we could avoid introduce the flow value. And essentially:

  • v-click (default, +1)
  • v-click="+0" == v-after
<div v-click>        show >= 1 </div>
<div v-click="'+2'"> show >= 3 </div>
<div v-click="'-1'"> show >= 2 </div>

I like that consistency 👍

@antfu antfu changed the title refactor: clicks system fix!: rework the clicks system Feb 9, 2024
@KermanX
Copy link
Member Author

KermanX commented Feb 9, 2024

新年快乐~

The ba9d9ab commit refactored the clicks context, and fixed Presenter and Print mode.

Currently, there is only one injection value about the clicks system: injectionClicks: (Outdated)

interface ClicksContext {
  readonly disabled: boolean // Before this commit, it was `injectionClicksDisabled`
  readonly current: number // Current click num. Before this commit, it was `injectionClicks`
  readonly flow: ClicksFlow // Before this commit, it was `injectionClicksFlow`
  readonly map: ClicksMap / Before this commit, it was `injectionClicksMap`
  register: (el: ClicksElement, resolved: ResolvedClicksInfo) => void // Register an element to the clicks flow and map
  unregister: (el: ClicksElement) => void // Unregister an element
  readonly flowSum: number // sum(...flow.values())
  readonly total: number // Math.max(...[...map.values()].map(v => v.max))
}

Currently, there is one restriction: One element can only be provided with the same clicksContext from the start to the end. For a main slide, the clicksContext is stored on its route.meta.__clicksContext. (Maybe this restriction can be removed. But I've tried for several hours and finally gave up. I think the main difficulty is that the mutation of states in v-click directives is not a typical Vue usage, which messed up the reactivity)

Check the state of a element:

const clicks = inject(injectionClicks)!.value
clicks.map.get(el) // => ResolvedClicksInfo
// @slidev/types
export interface ResolvedClicksInfo {
  max: number // The max required clicks num. Used to automatically calculate the clicks num of the slide.
  flowSize: number // The "duration" of the element in the flow. For absolute elements, it is zero.
  isCurrent?: ComputedRef<boolean> // CLASS_VCLICK_CURRENT
  isActive?: ComputedRef<boolean> // For `v-click`, active means it is visible; For `v-click-hide`, active means it is hidden.
  shows?: ComputedRef<boolean> // Is this element visible?
}

@KermanX KermanX force-pushed the refactor-clicks branch 2 times, most recently from 0c0afda to 792e2b8 Compare February 10, 2024 09:23
@KermanX KermanX marked this pull request as ready for review February 10, 2024 09:48
Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

Overall great refactors! Thanks a lot!

packages/client/logic/clicks.ts Outdated Show resolved Hide resolved
packages/client/builtin/KaTexBlockWrapper.vue Outdated Show resolved Hide resolved
packages/client/internals/SlideWrapper.ts Show resolved Hide resolved
packages/client/internals/SlidesOverview.vue Outdated Show resolved Hide resolved
packages/client/modules/directives.ts Outdated Show resolved Hide resolved
packages/client/logic/clicks.ts Outdated Show resolved Hide resolved
packages/types/src/types.ts Outdated Show resolved Hide resolved
packages/client/logic/nav.ts Outdated Show resolved Hide resolved
packages/client/logic/nav.ts Show resolved Hide resolved
return {
nav: {
...nav,
...navClicks,
Copy link
Member

Choose a reason for hiding this comment

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

I feel we might still need this, that clicks, clicksTotal, clicksContext should resolved from injections (as we have primary clicks and static clicks now). Components calling this function should get the context to the state of that slide

Copy link
Member Author

@KermanX KermanX Feb 13, 2024

Choose a reason for hiding this comment

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

I found this in the docs: (https://sli.dev/custom/vue-context#slidev-nav)
image

After a41a4f4, this PR behaves the same as what the doc says.

Should we get $slidev.nav.clicks, $slidev.nav.clicksTotal, $slidev.nav.clicksContext from the injections, and update the docs?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, good point. Yeah, I think it's better to use the injection and fix this, yes

@KermanX KermanX marked this pull request as draft February 12, 2024 12:17
@KermanX
Copy link
Member Author

KermanX commented Feb 12, 2024

Fixed in 9586e76

I've just found that HMR still breaks the clicks system. This is because the same CodeBlockWrapper is mounted twice with two different new elements every HMR. The onMounted hook is called twice, but only the second one's onUnmounted hook is called in the next HMR, which causes the problem. I am wondering if it is a Vue bug, but I can't create a minimal repro.

Only the second one's element can be seen in the DOM tree after HMR.

I've tried const vm = getCurrentInstance() + onUnmouted(..., vm), but the issue still happens.

@KermanX KermanX marked this pull request as ready for review February 13, 2024 09:24
@antfu
Copy link
Member

antfu commented Feb 13, 2024

That's a really nice PR + the description you have! Certainly don't worry about the English writing - you are doing great!

Overall looks good to me, let's merge it and ship as a beta to test out how it goes. Thank you!

@antfu antfu merged commit b814479 into slidevjs:main Feb 13, 2024
8 checks passed
@KermanX
Copy link
Member Author

KermanX commented Feb 13, 2024

Thank you for the encouragement. I will try my best to update the docs as soon as possible.

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