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

refactor(library locking): migrate files to TypeScript #5405

Merged
merged 13 commits into from
Jan 6, 2025

Conversation

magicznyleszek
Copy link
Member

@magicznyleszek magicznyleszek commented Jan 2, 2025

💭 Notes

Migrating files to TS introduced some non-trivial changes, like:

  • enum introduction required some parts of the code to be rewritten (more if's etc.)
  • mocks file had to be rewritten from scratch, as the asset responses there were extremely outdated and incomplete

👀 Preview steps

There is a nice long Library Locking Technical Reference article that I feel would be very helpful here :)

Some useful templates for testing:

Testing:

  1. Upload one of the above templates (no drag&drop!) using "My Library" > "NEW" > "Upload file" with "Upload as template" checked (important! as without this, all locking features would be stripped by BE code).
  2. Open the template in Form Builder
  3. 🟢 notice that locking features work as expected (see Technical Reference linked above)
  4. Go to "My Library".
  5. For the locked template in the list use "Create project" button.
  6. Open the newly created project in Form Builder.
  7. 🟢 notice that locking features work as expected

There are three fixes in this PR (mea culpa for not splitting).

First fix: I changed how isAssetLockable works, by extending it to also include templates (previously only allowed surveys). This function works like a feature flag -ish, and I've noticed that with a template that is fully locked (has lock_all: true), the Form Builder sidebar wasn't all disabled.

Second fix: in the same area as above, Background audio setting was not being disabled, and it should be with lock_all. I suspect we've added locking before adding Background audio in there and forgot to include it.

Third fix: I no longer disabled "Add from Library" and "Layout & Settings" buttons for lock_all forms. There is an annoying tiny UX bug that was happening with current code:

  1. Open a non-locked form in Form Builder and open "Layout & Settings" sidebar
  2. The sidebar being opened is being remembered by FE code
  3. Open a locked form in Form Builder and notice the sidebar is open
  4. 🔴 Notice you can't close the sidebar, because the toggle button is disabled
  5. 🟢 Notice that with this PR it is now possible to close sidebar (and everything inside the sideabar is already disabled, so no harm if user opens it)

@magicznyleszek magicznyleszek self-assigned this Jan 4, 2025
@magicznyleszek magicznyleszek marked this pull request as ready for review January 4, 2025 12:10
Copy link
Contributor

@Akuukis Akuukis left a comment

Choose a reason for hiding this comment

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

LGTM, not tested.

See 3 pushed commits - I started a review but quickly figured that it's much easier to show than tell:

  • 1st commit - fully used enum elsewhere, and ended up removing the object
  • 2nd commit - don't use let
  • 3rd commit - see how satisfies AssetResponse instead of : AssetResponse yields more appropriate typings for the mocks and eliminates the need for the redundant type-guards.

Copy link
Contributor

@pauloamorimbr pauloamorimbr left a comment

Choose a reason for hiding this comment

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

LGTM! 👌🏻

@magicznyleszek magicznyleszek merged commit 522a775 into main Jan 6, 2025
7 checks passed
@magicznyleszek magicznyleszek deleted the leszek/typescriptize-locking-files branch January 6, 2025 16:39
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.

3 participants