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 glitchy UI dropdown select for max downloads and expiration #36

Merged
merged 7 commits into from
May 19, 2021

Conversation

pirate
Copy link

@pirate pirate commented May 19, 2021

Currently there is a bug with the UI selection dropdowns where config values EXPIRE_TIMES_SECONDS and DOWNLOAD_COUNTS loaded as string arrays fail the strict if (existing val !== chosen val) set... check in the dropdown onchange handler https://github.com/timvisee/send/pull/36/files#diff-a9e45594669203e5e550875e43529ae26c24aeffb9af1bb0ba333015f471447cL15-R23, causing this glitchy behavior:

bug screen recording

There is no way to provide the EXPIRE_TIMES_SECONDS and DOWNLOAD_COUNTS config values as integer arrays with Docker, so this PR fixes the handling in the case that they are string arrays (as they always are when set via env vars). It also adds some logging to make it clearer in the console and from the HTML which item is selected and fixes a race condition between the form selection change and signup modal popup.

PR Roadmap:

  • record existing master behavior with default config value: works as expected, dropdowns chose values and do not reset on change
  • record existing master behavior with config values changed via env var to string array: broken, dropdowns reset on change (see screen recording)
  • confirm behavior of this PR using default/unset config values is correct: TODO
  • confirm behavior of this PR with config values changed via env var to string array is correct: TODO
  • confirm behavior of this PR is consistent across all major browsers (Chrome/Firefox/Safari): TODO
  • change from draft -> ready for review and get an approval from timvisee
  • merge and release 🎉 🚀

Copy link
Owner

@timvisee timvisee left a comment

Choose a reason for hiding this comment

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

Could you apply these two changes on your branch (or 4)?

app/ui/expiryOptions.js Outdated Show resolved Hide resolved
app/ui/expiryOptions.js Outdated Show resolved Hide resolved
@pirate pirate marked this pull request as ready for review May 19, 2021 09:15
@pirate
Copy link
Author

pirate commented May 19, 2021

Ok applied the changes. I haven't tested the latest version yet myself though, so don't merge it immediately. If you have a chance to test it and it works, feel free to merge it. Otherwise, I'll post back in a day or two once I've had a chance to spin up a dev server and test it across a few browsers and config setups.

@timvisee
Copy link
Owner

I'll try to give it a spin soon.

@timvisee
Copy link
Owner

I've tested the branch with Firefox and Chrome, which seems to work fine.

The new config function did not parse a string array correctly, but I'll fix that on master.

Thanks!

timvisee added a commit that referenced this pull request May 19, 2021
@timvisee timvisee merged commit 1d6872e into timvisee:master May 19, 2021
@timvisee
Copy link
Owner

I've released this as part of v3.4.11.

It should be available when this pipeline succeeds: https://gitlab.com/timvisee/send/-/pipelines/305718044

@pirate
Copy link
Author

pirate commented May 19, 2021

Awesome, thanks!

@pirate pirate deleted the patch-3 branch May 20, 2021 16:39
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.

2 participants