-
Notifications
You must be signed in to change notification settings - Fork 126
DT-3069: Add search attributes form component with adapter pattern #2793
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
DT-3069: Add search attributes form component with adapter pattern #2793
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
106fbc3 to
ad40516
Compare
5b77f50 to
f72b56f
Compare
Implements a search attributes management form using SuperForms and Svelte 5 runes. The component features error handling, form state management, and an adapter pattern for different API implementations. Key implementation details: - SuperForms integration with Zod validation and error callbacks - Adapter pattern with duck-typed interfaces for OSS/Cloud flexibility - Error handling with user-friendly messages - Support for both field-level and form-level validation display - Skeleton loading states and retry functionality for failed operations The adapter pattern abstracts API differences between OSS and Cloud implementations, with placeholder methods for future CRUD endpoints when the SDK team adds them.
861da83 to
cbcf49b
Compare
Streamlined documentation to focus on actionable patterns and rules rather than verbose explanations. Condensed Svelte 5 runes patterns into concise examples and reduced overall content to key workflow commands and coding standards.
cbcf49b to
b6794a5
Compare
Adds i18n support for all user-facing strings in the search attributes form feature. Creates new search-attributes namespace with strings for form labels, validation messages, error states, and data type labels. Key changes: - New search-attributes i18n namespace with 25+ strings - All form components updated to use translate() function - Adapter classes use i18n for type labels and error messages - Storybook stories internationalized - Validation messages and success/error states localized - Data type labels (Keyword, Text, Int, etc.) centralized All hardcoded strings replaced with proper i18n keys, making the feature ready for multi-language support.
Update font sizes to match Figma design specs: - Set story template header to 14px (text-sm) - Set "Add New Custom Search Attribute" button to 14px (size="sm")
Add count badge to save button showing number of unsaved changes using SuperForms isTainted state. Badge displays count of modified attributes and only appears when form has unsaved changes. - Use SuperForms tainted and isTainted for dirty state detection - Add Badge component with count type for numbered indicator - Count only attributes with actual field changes (ignore empty objects) - Position badge using established design system pattern
|
|
||
| export class DefaultSearchAttributesAdapter implements SearchAttributesAdapter { | ||
| constructor( | ||
| private namespace: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to think of a situation we'd want to pass in the namespace versus just using page.params.namespace. Does it make sense to use a .svelte.ts here and use that as the default value so you don't have to pass in a namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, its more a separation of concerns thing. the Adapter doesn't need to know about the page context. But, I'm fine either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He is an example of using svelte.ts and page state. Also moved the onSuccess and onCancel callbacks into the adapter.
64d1051#diff-296750d064a1071282c7efa6ed1df190fa9cc1ffee30426d27f71d6154b39170
| await adapter.upsertAttributes(form.data.attributes); | ||
| onSave(form.data.attributes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might cause some confusion on what upsertAttributes is responsible for versus what onSave does. Usually onSave functions are handing the api call. Is this more of an onSave callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The adapter method is responsible for persisting the changes. AKA, API call. But the onSave handles the result of the apiCall. onSave should handle toast messages or redirects, page related stuff. It's more of an onSuccess callback and maybe should be renamed as such. The intention was to keep the adapter dumb and the config kept internal to the reusable component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 sounds good, I like onSuccess
Refactor adapter to use Svelte reactive context instead of constructor parameters: - Replace class-based adapter with reactive object using page.params - Move callbacks into adapter definition removing prop drilling - Create new route page demonstrating zero-config usage - Update all mock adapters and stories to match new pattern - Add proper form reset functionality using SuperForms reset() - Update documentation to reflect simplified API The adapter now automatically gets namespace from URL params and requires no setup.
Replace class-based mock adapters with simple object literals similar to the default adapter pattern. This eliminates confusion and clearly demonstrates the intended usage pattern for creating custom adapters. - Convert MockAdapter, ErrorMockAdapter, and EmptyMockAdapter from classes to objects - Remove complex constructor patterns and parameter passing - Use inline callback definitions matching defaultAdapter approach - Maintain all functionality while improving clarity
Fix outdated Storybook import paths that were causing dev server errors. Update from deprecated addon-docs/blocks to @storybook/blocks for Storybook 8.x compatibility.
55c2f4b to
97fdcb8
Compare
Update MDX documentation to reflect the simplified reactive adapter pattern: - Document zero-configuration default adapter usage - Show object-based custom adapter examples instead of classes - Add dirty state management section with SuperForms isTainted - Update usage examples with correct import order - Document i18n support and new features - Simplify props table removing callback parameters
Minor spacing and line break adjustments for better readability.
Description & motivation 💭
This PR introduces a reusable search attributes form component that addresses the need for a standardized way to manage custom search attributes across different Temporal environments (OSS and Cloud). The component provides a consistent user experience for creating, editing, and validating search attributes while handling the underlying API differences transparently.
Motivation:
Key User Benefits:
Design Considerations 🎨
The component follows the established Holocene design system patterns and integrates seamlessly with existing UI components. Form validation provides immediate feedback for common issues like duplicate attribute names, while error states are clearly communicated through alert components.
Testing 🧪
How was this tested 👻
Manual Testing:
Storybook Documentation:
Steps for others to test: 🚶🏽♂️🚶🏽♀️
Checklists
Draft Checklist
Merge Checklist
Issue(s) closed
Closes DT-3069
Docs
Any docs updates needed?