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(ui) Followups to recent changes to UI ingestion forms #5602

Conversation

chriscollins3456
Copy link
Collaborator

We recently merged in a big change to ingestion forms in the UI and this PR addresses some followups after that.

  • Pass in removeMargin optional prop for styling to SecretField
  • Add typescript typing to our RECIPE_FIELDS object
  • Simplify value setting logic by allowing our main value setter to work for fieldPaths that are both strings and a list of strings (for field keys that are dotted ie. sasl.username)
  • Update some incorrect tooltips

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@chriscollins3456 chriscollins3456 changed the title fix(ui) Small followups to recent changes to UI ingestion forms fix(ui) Followups to recent changes to UI ingestion forms Aug 9, 2022
@github-actions
Copy link

github-actions bot commented Aug 9, 2022

Unit Test Results (build & test)

499 tests  ±0   499 ✔️ ±0   8m 15s ⏱️ -5s
115 suites ±0       0 💤 ±0 
115 files   ±0       0 ±0 

Results for commit fcd2415. ± Comparison against base commit 2b2488e.

♻️ This comment has been updated with latest results.

Comment on lines -11 to -12
setValueOnRecipeOverride: (recipe: any, values: string[]) =>
setDottedFieldValuesOnRecipe(recipe, values, saslUsernameFieldPath),
Copy link
Contributor

Choose a reason for hiding this comment

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

nice to be able to simplify this!

@@ -74,7 +75,18 @@ import {
TOPIC_DENY,
} from './kafka';

export const RECIPE_FIELDS = {
interface RecipeFields {
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, why did you opt for interface here? I usually use type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so as far as I understand, the two are very often interchangeable. I usually default to interface because interface names show up in error messages which helps for debugging and have some extra functionality like extending (not that I'm using that here). I have always used type for basically giving a name to a primitive type for readability.

I don't think either are wrong and neither is the obvious right choice either!

Copy link
Contributor

@gabe-lyons gabe-lyons left a comment

Choose a reason for hiding this comment

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

are the dotted fields only able to be provided that way? so if you put

sasl:
  username: true

that would be considered invalid in the kafka recipe?

@anshbansal anshbansal added the product PR or Issue related to the DataHub UI/UX label Aug 10, 2022
@chriscollins3456
Copy link
Collaborator Author

chriscollins3456 commented Aug 10, 2022

are the dotted fields only able to be provided that way? so if you put

sasl:
  username: true

that would be considered invalid in the kafka recipe?

@gabe-lyons I'm actually not positive. I'd like to think this would still be considered valid, but I never dug too deep into it. I just know in our docs and example recipe we have the dotted key like sasl.username: {value} so I just decided to keep that convention when building through the form

@jjoyce0510 jjoyce0510 merged commit 626196d into datahub-project:master Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants