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

Add double check for 'keepFocus' and 'noScroll' in goto to allow for backcompatibity #7678

Merged
merged 9 commits into from
Nov 16, 2022
Merged

Conversation

paoloricciuti
Copy link
Member

It's a very simple change. It avoids throwing an error when keepfocus or noscroll it's in the opts of goto if there are also the correct case inside the opts. This should allow for back compatibility for library writers because they can pass both.

I wasn't able to run test for this.


Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Nov 16, 2022

🦋 Changeset detected

Latest commit: 691c9a9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Conduitry
Copy link
Member

I don't know whether you did something with line endings, but every single line appears as changed in Git.

@dummdidumm
Copy link
Member

Which library depends on this and wants to be able to use both? Why doesn't the library do a major version bump (or a minor if in prerelease mode)?

@Conduitry
Copy link
Member

This change doesn't make sense to me. Why only throw an error if both the old and new form are set? Wouldn't this mean that someone who still only has nofocus set would no longer get an error but also their code would silently not work they way they were expecting?

@paoloricciuti
Copy link
Member Author

I don't know whether you did something with line endings, but every single line appears as changed in Git.

My bad it was line endings, i fixed it now

Which library depends on this and wants to be able to use both? Why doesn't the library do a major version bump (or a minor if in prerelease mode)?

I have a library that i've already fixed with the new case and bumped the version. But this way if an user uses my library (or some other library that want to mantain backcompatibility) they can use it either if they use a newer version of kit or an older one. And it also seems much more reasonable to check if both case exist instead of just one.

@paoloricciuti
Copy link
Member Author

paoloricciuti commented Nov 16, 2022

This change doesn't make sense to me. Why only throw an error if both the old and new form are set? Wouldn't this mean that someone who still only has nofocus set would no longer get an error but also their code would silently not work they way they were expecting?

It'll throw if there's no keepFocus AND there's keepfocus instead of throwing only if there's keepfocus. Basically if you have both it will not throw. If you have keepfocus without keepFocus it will throw.

Here's the table of truth

keepFocus keepfocus !keepFocus && keepfocus
0 0 0
0 1 1
1 0 0
1 1 0

@Conduitry
Copy link
Member

Oh, I missed the !. That's not the right syntax, though. ! is higher precedence than in, so you need some parentheses. Have you actually tried this change?

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

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

this addresses @Conduitry's response and fixes the typechecking failure

@Rich-Harris
Copy link
Member

please fix the indentation. the diff should be 2 lines, not 1,726!

@paoloricciuti
Copy link
Member Author

Oh, I missed the !. That's not the right syntax, though. ! is higher precedence than in, so you need some parentheses. Have you actually tried this change?

I tested the formula but i'm an idiot and i tested the wrong thing. Fixed now...i'm trying to fix line endings now.

@paoloricciuti
Copy link
Member Author

please fix the indentation. the diff should be 2 lines, not 1,726!

I finally did it...sorry.

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

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

it's failing CI because you didn't run pnpm format

@Rich-Harris Rich-Harris merged commit a762a09 into sveltejs:master Nov 16, 2022
@Rich-Harris
Copy link
Member

we got there in the end — thanks!

@paoloricciuti
Copy link
Member Author

we got there in the end — thanks!

Thank for your patience...did this rapidly with GitHub.dev but it got me in more trouble than cloning apparently lol

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.

4 participants