fix(PropertiesModal): do not show validation errors while loading

[msyavuz]

SUMMARY

Dashboard properties modal was showing validation errors when in a loading state. This pr fixes that.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

dashboard-properties-before.mp4

After:

dashboard-properties-after.mp4

TESTING INSTRUCTIONS

Visual testing

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[korbit-ai]

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Status
	Stale form value in error calculation ▹ view	✅ Fix detected

Files scanned

​

File Path	Reviewed
superset-frontend/src/dashboard/components/PropertiesModal/sections/BasicInfoSection.tsx	✅
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

[mistercrunch]

Oh thanks for fixing this. Wondering if it would make more sense to add a loading state for the reusable modal instead. I think I shifted things so all properties modal use the same component, so maybe we could handle there instead of handling in the first section/form elements. Modal would need the context though as to whether it's loading, but should be doable no?

Thinking further, wondering if we pop-fast with a spinner in-modal, or whether we spinner the trigger (trigger becomes a spinner, wait until it's loaded to pop the modal). Fine with either approach, spinner-in-modal is probably easier/best (?)

[msyavuz]

I think 'time to be interactive' is too low to warrant a loading spinner in modals, i will play with this some more and try to get rid of the loading state alltogether, if not yes could probably add loading to the underlying component.

[mistercrunch]

My main thing would be to handle this through a common solution/UX for all properties modal, so it's DRY and consistent across modals. About loading time, you never really know what could be happening, backend hiccup, super slow connection, ...

I hate loading-related flickering, let's find a solution that that minimizes it. The loading-state-in-trigger might be decent (antd has loading states for buttons). Otherwise was thinking a spinner inside the full-size modal might be ok.

[mistercrunch]

LGTM