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

🪟 🎉 Replace multiline + hidden connector field with SecretTextArea component #16539

Merged
merged 9 commits into from
Oct 5, 2022

Conversation

edmundito
Copy link
Contributor

@edmundito edmundito commented Sep 9, 2022

What

Resolves #16307

This change updates the rendering of multiline + secret connector field with a customized SecretTextArea component that handles the text more securely:

Screen.Recording.2022-09-09.at.12.12.55.mov

How

SecretTextArea stores the value inside a hidden <input type="password" /> field until it becomes visible. The data is then moved to a textarea for editing.

The component is wrapped around a TextInputContainer element which applies the correct styles to make it like the other fields. This new component could also be used for the Input and TextArea components.

Recommended reading order

  1. SecretTextArea.tsx
  2. Control.tsx
  3. Rest of the code

WIP - TODO

  • Finalize design
  • Fix rendering glitches
  • Unit testing for SecretTextArea

@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Sep 9, 2022
@edmundito edmundito marked this pull request as ready for review September 20, 2022 14:44
@edmundito edmundito requested a review from a team as a code owner September 20, 2022 14:44
Comment on lines 68 to 70
onClick={() => {
toggleIsContentVisible();
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit/Optional:

Suggested change
onClick={() => {
toggleIsContentVisible();
}}
onClick={toggleIsContentVisible}

onBlur?: React.FocusEventHandler<HTMLDivElement>;
}

export const TextInputContainer: React.FC<TextInputContainerProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to extend this component's usage? Because the secret text area only uses some of these props and it looks like it isn't used elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this will eventually be shared by the input component but the change will be done separately.

@krishnaglick
Copy link
Contributor

Tested functionality locally in OSS: Postgres source, SSH setting at the bottom

@@ -89,6 +89,7 @@ export const Input: React.FC<InputProps> = ({ light, error, ...props }) => {
{
[styles.disabled]: props.disabled,
[styles.password]: isPassword,
"fs-exclude": isPassword,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we maybe make this class an export from some (ts) file, to have it more verbose in those places what it does? Alternatively I'd suggest we add a comment here, so that if we read that in the future, won't need to google what that class does :)

@@ -0,0 +1,21 @@
@use "../../../scss/colors";

button.toggleVisibilityButton {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we usually have classNames only, is there a specific reason we're going with button.toggleVisibilityButton here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No good reason. I may have done this during the early commits and forgot to remove it.

extends Omit<TextInputContainerProps, "onFocus" | "onBlur">,
React.TextareaHTMLAttributes<HTMLTextAreaElement> {}

export const SecretTextArea: React.VFC<SecretTextAreaProps> = ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export const SecretTextArea: React.VFC<SecretTextAreaProps> = ({
export const SecretTextArea: React.FC<SecretTextAreaProps> = ({

So we're not introducing new deprecated API with the React 8 upgrade please (also see this Slack message).

className={styles.passwordInput}
readOnly
aria-hidden
data-testid="input"
Copy link
Collaborator

@timroes timroes Sep 23, 2022

Choose a reason for hiding this comment

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

Could we maybe make those testids a bit more verbose, e.g. data-testid="secretTextArea-input", that would help us later to better target them (in any tool) if we'd need to. (And similar for the testid above)

@timroes
Copy link
Collaborator

timroes commented Sep 23, 2022

I ❤️ this approach. I think this will really improve this component significantly.

@edmundito edmundito merged commit dfaf7d8 into master Oct 5, 2022
@edmundito edmundito deleted the edmundito/secret-textarea branch October 5, 2022 10:14
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
…component (airbytehq#16539)

* Add SecretTextArea component

* Add TextInputContainer to manage the look and feel of inputs and text area
* Update multiline + secret controls to use it

* Fix SecretTextArea story

* Add .fs-exclude class to SecretTextArea and Password input

* Use scss color

* Add tests for SecretTextArea component

* Update SecretTextArea testIds

* Remove specificity in secrettextarea rule

* Update type to be compatible with React 18

* Move SecretTextArea and TextInputContainer to components/ui
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSH Tunnels: Private SSH key not treated as secret by UI
3 participants