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

feat!: injection chains to allow merging custom and addons/themes keyboard shortcuts #702

Merged
merged 4 commits into from Sep 12, 2022

Conversation

twitwi
Copy link
Contributor

@twitwi twitwi commented Sep 3, 2022

Fixes #629

IMPORTANT, There are 4 commits

  • the 1st one is the core feature, probably without big issues,
  • the 2nd one is an (optional but that I find nice) refactoring,
  • the 3rd one is to be discussed (or just dropped) as it is a breaking change and would necessarily require a documentation update (that would be highly recommended anyways even with the first commits).
  • following the discussion commit 4 adds a user message to help migration

Basically, the third commit breaks every situation that used setup/shortcuts (the default shortcuts are then removed).
The doc update would suggest something like:

export default defineShortcutsSetup((nav: NavOperations, baseShortcuts: any) => {
  return [
    ...baseShortcuts,
    {

Related to #659 (authored by @manniL )

  • I kept the multi-args logic even if (with the 3rd commit) it is not used anymore, because it can become useful
  • to remove some shortcuts, the principle would be to remove from baseShortcuts before the return with ...baseShortcuts (i.e. there is no "global" object that we mutate, only the return value is used)

@twitwi twitwi requested a review from antfu September 3, 2022 21:52
@twitwi
Copy link
Contributor Author

twitwi commented Sep 3, 2022

Following the failing cypress, I'll force push the replacement of replaceAll by replace.

BREAKING CHANGE: Previous code that redefines shortcuts will remove all default ones.
@twitwi twitwi changed the title feat: injection chains to allow merging custom and addons/thems keyboard shortcuts feat: injection chains to allow merging custom and addons/themes keyboard shortcuts Sep 4, 2022
@twitwi twitwi added the question Further information is requested label Sep 4, 2022
@antfu
Copy link
Member

antfu commented Sep 4, 2022

I am not sure if the breaking change is necessary, but @manniL since you are using the feature, I guess we could have it if it make sense to you

@manniL
Copy link
Contributor

manniL commented Sep 4, 2022

I'm fine with the suggested changes as they put defining shortcuts a little bit more "in line" with other features, especially regarding "implicit" mutation of the shortcuts object ☺️

@twitwi
Copy link
Contributor Author

twitwi commented Sep 4, 2022

Another solution, which is better for backward compatibility and at the same time allow the new API... but is maybe over-engineered given than slidev is still in version 0.*... would be to keep the old API untouched and rename this one as setupShortcuts2 or something like that.

This could be combined with runtime messages and progressive deprecation... but again it is probably too much here.

The solution I'd (now) recommend is to do the breaking change + add a check: if all shortcuts have been removed (not returned by the injected function) then we warn the user (how?) with a link to the updated doc.

@twitwi
Copy link
Contributor Author

twitwi commented Sep 5, 2022

I proposed the doc update there slidevjs/docs#90
I still need to add a kind of warning message to this PR.

@twitwi
Copy link
Contributor Author

twitwi commented Sep 5, 2022

I think that combined with the docs PR this is a decent solution. I currently both alert and console.warn the message (to have a clickable link to the documentation).

@twitwi twitwi added enhancement New feature or request and removed question Further information is requested labels Sep 5, 2022
@twitwi
Copy link
Contributor Author

twitwi commented Sep 10, 2022

@antfu considering the discussion, I'd say this one is ok to merge together with the doc PR mentioned). As it is a breaking change, I leave you the final click.

@antfu antfu changed the title feat: injection chains to allow merging custom and addons/themes keyboard shortcuts feat!: injection chains to allow merging custom and addons/themes keyboard shortcuts Sep 12, 2022
@antfu antfu merged commit a067890 into slidevjs:main Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow both custom and addons/themes keyboard shorcuts
3 participants