-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🪟 🐛 Update connection name - fix minor issues #13550
🪟 🐛 Update connection name - fix minor issues #13550
Conversation
- unable to delete last chart in input - delete the whole name by 'select all' command - trim entered names - don't allow to save empty names
…sues # Conflicts: # airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ConnectionName.tsx # airbyte-webapp/src/utils/addEnterEscFuncForInput.tsx
@@ -102,7 +102,7 @@ const Input: React.FC<InputProps> = (props) => { | |||
}, [inputRef, defaultFocus]); | |||
|
|||
return ( | |||
<InputContainer {...props} className={focused ? "input-container--focused" : undefined}> | |||
<InputContainer className={focused ? "input-container--focused" : undefined}> | |||
<InputComponent | |||
{...props} |
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.
@edmundito need your advice. Here we pass the props to InputContainer
and InputComponent
. Which causes firing onChange
twice. Do we really need to rethrow props to InputContainer
?
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.
Great that you figured out why onChange
was firing twice. No, we do not need it. All we need is disabled
for the theme to work correctly.
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.
Nice fixes. I did notice that we are able to edit the Connection name when the Connection is in the middle of a Sync. When you do so, unfocusing the field doesn't reflect the "new" name (you have to refresh the page to see it). I wonder if we should be able to edit the name during a sync or not. If so, we should probably tidy that up!
@tealjulia Good question! Need to polish this feature as much as we can 🔥 |
I think editing the name of a connection is unrelated to sync jobs that might be in-progress, so I don't think of that as an issue. |
- replace "props: any" with right types - rename 'addEnterEscFuncForInput' to more general and self-describable name
A little bit polished the feature and pushed some updates:
|
@@ -102,7 +102,7 @@ const Input: React.FC<InputProps> = (props) => { | |||
}, [inputRef, defaultFocus]); | |||
|
|||
return ( | |||
<InputContainer {...props} className={focused ? "input-container--focused" : undefined}> | |||
<InputContainer className={focused ? "input-container--focused" : undefined}> | |||
<InputComponent | |||
{...props} |
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.
Great that you figured out why onChange
was firing twice. No, we do not need it. All we need is disabled
for the theme to work correctly.
airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ConnectionName.tsx
Outdated
Show resolved
Hide resolved
…sues # Conflicts: # airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ConnectionName.tsx
…sues # Conflicts: # airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ConnectionName.tsx
… order issue" This reverts commit 81d7ba4
What
Closes #13416
How
Recommended reading order
Input.tsx
- there was an issue in the base Input component. With extra props drilling which caused firing 'onChange' twiceConnectionName.tsx