fix(database-modal): Show a different placeholder text in Snowflake connection form #21172
Conversation
When adding a new database and selecting Snowflake, the database and account fields had the same placeholder. This PR adds a placeholder prop so values can be sent dynamically by field
There was a problem hiding this comment.
Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️
We hope to see you in our Slack community too!
| @@ -1607,6 +1615,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({ | |||
| } | |||
| getValidation={() => getValidation(db)} | |||
| validationErrors={validationErrors} | |||
| getPlaceholder={field => getPlaceholder(field)} | |||
There was a problem hiding this comment.
Could this be done without defining a new inline function each render?
| getPlaceholder={field => getPlaceholder(field)} | |
| getPlaceholder={getPlaceholder} |
| @@ -85,10 +86,10 @@ export const databaseField = ({ | |||
| value={db?.parameters?.database} | |||
| validationMethods={{ onBlur: getValidation }} | |||
| errorMessage={validationErrors?.database} | |||
| placeholder={t('e.g. world_population')} | |||
| placeholder={t(placeholder ?? 'e.g. world_population')} | |||
There was a problem hiding this comment.
I don't have a great sense of how translation strings work, but I wonder if this might break the build step of extracting translatable strings to have the argument of t() be dynamically set at render time. If you or someone else knows for sure definitely correct me, but would it be safer to have this just be placeholder ?? t('e.g. world_population') and instead call t() for placeholder where the string literal is defined in superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx?
Codecov Report
@@ Coverage Diff @@
## master #21172 +/- ##
==========================================
- Coverage 66.37% 66.36% -0.01%
==========================================
Files 1781 1781
Lines 67871 67890 +19
Branches 7239 7248 +9
==========================================
+ Hits 45047 45057 +10
- Misses 20966 20971 +5
- Partials 1858 1862 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Storybook has completed and can be viewed at |
…wflake connection form (apache#21172)" This reverts commit da3401a.
When adding a new database and selecting Snowflake, the database and account fields had the same placeholder. This PR adds a placeholder prop so values can be sent dynamically by field
SUMMARY
Adds an option to set a dynamic placeholder based on field.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION