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(web): admin settings number input validation #9470

Merged
merged 3 commits into from
May 14, 2024

Conversation

michelheusschen
Copy link
Contributor

Fixes #9361 by performing input validation on:blur instead of on:input

Copy link

cloudflare-pages bot commented May 14, 2024

Deploying immich with  Cloudflare Pages  Cloudflare Pages

Latest commit: ac7ef2c
Status: ✅  Deploy successful!
Preview URL: https://5d7b9eaf.immich.pages.dev
Branch Preview URL: https://fix-web-admin-settings-numbe.immich.pages.dev

View logs

@@ -0,0 +1,30 @@
import { render } from '@testing-library/svelte';
import userEvent from '@testing-library/user-event';
// @ts-expect-error import works
Copy link
Contributor

@zackpollard zackpollard May 14, 2024

Choose a reason for hiding this comment

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

This should be unnecessary, I pulled the branch locally and after an npm install the error went away, lets remove this then I am good to merge it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The import does work, only typescript complains about it. Initially @ts-expect-error wasn't included but that failed: https://github.com/immich-app/immich/actions/runs/9078498613/job/24945653995

There are three possible solutions I came up with:

  1. Move <script lang="ts" context="module"> to a dedicated .ts file and change all imports
  2. Manually declare the type in module '*.svelte'
  3. Just ignore the error

I chose for the easy option 3, but if another solution is desirable we can also do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I see that now, why that doesn't error in the IDE that also runs typescript against the file, I have no idea...

Copy link
Contributor

Choose a reason for hiding this comment

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

The annoying part for me is that the ts-expect-error line errors because my IDE doesn't believe there is actually an error there 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

I have only seen and run into issues when using <script lang="ts" context="module">. Personally, I just prefer to externalize it into a dedicated ts file.

@zackpollard zackpollard force-pushed the fix/web-admin-settings-number-input-validation branch from c5db8c1 to ac7ef2c Compare May 14, 2024 15:30
@zackpollard zackpollard enabled auto-merge (squash) May 14, 2024 15:31
@zackpollard zackpollard merged commit acc611a into main May 14, 2024
23 checks passed
@zackpollard zackpollard deleted the fix/web-admin-settings-number-input-validation branch May 14, 2024 15:35
@NicholasFlamy
Copy link
Contributor

Thank you, thank you, thank you! This is a huge relief. It's working great.

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

Successfully merging this pull request may close these issues.

Web admin settings page number input broken
4 participants