-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
🪟 🎨 Connector Field UX Improvements #16152
Conversation
@lmossman I updated Figma with a new component for the group of fields : https://www.figma.com/file/gMNRYVJav25pdsoxAyW7Qp/00_01_WEB-APP-LIBRARIES?node-id=1107%3A1880 - and here: https://www.figma.com/file/AxqFXuHo2BoTY0QY12hxm1/00_02_WEB-APP-COMPONENTS?node-id=446%3A8860 (you can change the width of this group of fields on Figma) |
@lmossman would it be possible to add storybooks for the areas impacted? |
Looks like there's a conflict! |
)} | ||
{props.optional && ( | ||
<Text size="sm" className={styles.optionalText}> | ||
Optional |
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.
Should we put this into a translatable string?
@@ -1,7 +1,7 @@ | |||
import React from "react"; | |||
import sanitizeHtml from "sanitize-html"; | |||
|
|||
interface IProps { | |||
interface IProps extends React.InputHTMLAttributes<HTMLInputElement> { |
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.
Two nits:
- We should change this to just
Props
- Could we get rid of the default export while you're in here?
airbyte-webapp/src/views/Connector/ServiceForm/components/Property/LabelInfo.module.scss
Outdated
Show resolved
Hide resolved
|
||
import styles from "./LabelInfo.module.scss"; | ||
|
||
interface IProps { |
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.
interface IProps { | |
interface Props { |
description?: string; | ||
} | ||
|
||
const LabelInfo: React.FC<IProps> = ({ label, examples, description }) => { |
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.
const LabelInfo: React.FC<IProps> = ({ label, examples, description }) => { | |
export const LabelInfo: React.FC<Props> = ({ label, examples, description }) => { |
); | ||
}; | ||
|
||
export { LabelInfo }; |
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.
export { LabelInfo }; |
} | ||
|
||
const LabelInfo: React.FC<IProps> = ({ label, examples, description }) => { | ||
const constructExamples = () => { |
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 should be a component. I'd argue for moving it above this one, renaming to Examples
perhaps, and passing it the props so the use should look like:
<Examples examples={examples} />
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 like that suggestion, thanks!
@@ -64,13 +65,12 @@ export const ArraySection: React.FC<ArraySectionProps> = ({ formField, path, dis | |||
}; | |||
}, [items, formField.properties]); | |||
|
|||
const label = ( |
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.
Nit: I'd either move this down into title={<Property...
or call this groupTitle
or something.
fullWidthTitle | ||
title={ | ||
<div className={styles.sectionTitle}> | ||
{label ? <GroupLabel>{label}:</GroupLabel> : null} | ||
{label ? <GroupLabel>{label}</GroupLabel> : null} |
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.
Could label ever be undefined now?
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.
Doesn't seem like it -- I've removed the conditional here
@@ -24,7 +25,7 @@ const GroupLabel = styled(Label)` | |||
`; | |||
|
|||
const ConditionControls = styled.div` | |||
padding-top: 25px; | |||
padding-top: 20px; | |||
`; |
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.
Since we have a styles folder, we should get rid of ConditionControls and just use a style.
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.
Really great work here Lake!
className?: string; | ||
} | ||
|
||
const PropertyLabel: React.FC<React.PropsWithChildren<PropertyLabelProps>> = ({ |
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.
Should export here, not below.
* use Source name and Destination name * move label messages into info tooltips * pass tooltip props through IntoTooltip and set placement * add error message to ControlLabels * refactor to reduce duplication and add optional indicator * add label as header in info tooltip and fix tooltip word wrapping * remove placeholders and style tooltip a bit better * make example background darker * remove unnecessary style * put examples in flex box * use property label in labeled switch * use info tool tip for condition section * remove unused class name * remove colon * add styling to lists inside of tooltip * move errors below inputs and add error flag to control input * remove label css * use new property label for array section as well * fix css * make examples a bit more circular * put optional text into translatable string * fix TextWithHTML props and export * simplify CSS selector * rename IProps to Props * fix export * move examples into its own component * remove unnecessary var * conditionSection feedback * update design of group sections * fix empty group control styling * add comment * fix position and sizing of group control sections * adjust margin * add main changed components to storybook * linting fixes * fix group section styling and clean up storybook * fix stylelint errors * wrap storybook components in cards * use card prop to fix build error * fix info tooltip positioning and prevent wrapping on group label * adjust infotooltip styling again * fix info tooltip position on safari as well * fix styling of condition section dropdown * small spacing adjustment * another small padding adjustment * switch display to block * simplify scss classes * rename props class * fix casing of scss variables * use scss variables in some places * move PropertyError to Property folder * replace displayError with hasError * rename infoMessage to infoTooltipContent * move stories to other folders * fix display and local var * update snapshots * properly set htmlFor on group control label and fix nest group control styling * fix tooltip border radius * fix group controls border radius * fix spacing of field forms * fix input border and background colors * fix display types in property section * update snapshot classes
* use Source name and Destination name * move label messages into info tooltips * pass tooltip props through IntoTooltip and set placement * add error message to ControlLabels * refactor to reduce duplication and add optional indicator * add label as header in info tooltip and fix tooltip word wrapping * remove placeholders and style tooltip a bit better * make example background darker * remove unnecessary style * put examples in flex box * use property label in labeled switch * use info tool tip for condition section * remove unused class name * remove colon * add styling to lists inside of tooltip * move errors below inputs and add error flag to control input * remove label css * use new property label for array section as well * fix css * make examples a bit more circular * put optional text into translatable string * fix TextWithHTML props and export * simplify CSS selector * rename IProps to Props * fix export * move examples into its own component * remove unnecessary var * conditionSection feedback * update design of group sections * fix empty group control styling * add comment * fix position and sizing of group control sections * adjust margin * add main changed components to storybook * linting fixes * fix group section styling and clean up storybook * fix stylelint errors * wrap storybook components in cards * use card prop to fix build error * fix info tooltip positioning and prevent wrapping on group label * adjust infotooltip styling again * fix info tooltip position on safari as well * fix styling of condition section dropdown * small spacing adjustment * another small padding adjustment * switch display to block * simplify scss classes * rename props class * fix casing of scss variables * use scss variables in some places * move PropertyError to Property folder * replace displayError with hasError * rename infoMessage to infoTooltipContent * move stories to other folders * fix display and local var * update snapshots * properly set htmlFor on group control label and fix nest group control styling * fix tooltip border radius * fix group controls border radius * fix spacing of field forms * fix input border and background colors * fix display types in property section * update snapshot classes
What
Relates to #12204
This PR addresses the following parts of the above ticket:
error
prop logic on the inputs to make their border red:*
or lack-thereof after property labels which was meant to indicate optionality, and instead adds anOptional
text to the right of the label for any optional fields.*
or(Optional)
at the end - the connectors should be updated to remove these redundant suffixes now that we have logic in place to add theOptional
text after the field. Should I programmatically try to filter these strings out of the field names in the meantime?optional
flag on the PropertyLabel tofalse
for the LabeledSwitch, ConditionSection, and ArraySection components, because it felt confusing as a user to haveOptional
on each of these when there is not really a way to not use them -- the switch is always on or off, condition always has some dropdown value selected, and even if the array is empty it is still submitted as a value. So to me, optional only really seemed to make sense on the basic text input components.TODO: modify the styling on Condition and Array section components to more closely match the styling in the original design, with the label being fully left-justified. This will allow us to hide the bounding line when there are no children in the condition group, since that case currently looks a bit strange now that the description has been moved into the InfoTooltip, e.g.:Currently just waiting on a final confirmation of the design from Nico before implementing this. This could also be done in a follow-up PR if the current state in this PR is acceptable.
The styling of grouped fields has been updated to match the design provided by Nico:
NOTE: This PR also does not do any of the field grouping / optional fields hidden behind an expandable dropdown changes present in the design. Field grouping will require some more changes to connector specs, and hiding optional fields can be done in a follow-up PR.
How
There are several places that I have tried to refactor or simplify some components to reduce duplication and remove unneeded logic.
I also tried to make sure that the changes I've made here won't affect other views that are using the same components that I've touched, such as the connection Replication view. Though, the changes I've made here should hopefully make it easy to update those places to use the new styling as well (e.g. use the
PropertyLabel
class I have introduced to get the description-in-tooltip andOptional
flag styling)Recommended reading order
PropertySection.tsx
- changes to the base input propertiesPropertyLabel.tsx
- replacement of theairbyte-webapp/src/views/Connector/ServiceForm/components/Property/Label.tsx
component, since I was getting confused at the fact that it was named the same as theairbyte-webapp/src/components/Label/Label.tsx
component which is also used further down the component tree.LabelInfo.tsx
/LabelInfo.module.scss
- replacement of theLabelMessage
component, since this information is no longer being displayed in label "message" but instead in the InfoTooltip. Used by thePropertyLabel
component to display an InfoTooltip with the field description and examples.ControlLabels.tsx
- main change here is to add the InfoTooltip and theOptional
pieces after the field label, which are both configured through new props so that this does not modify other views that use this componentConnectorNameControl.tsx
- modified this to use the newPropertyLabel
to get the same info tooltip styling as other inputsControl.tsx
- just removed the example-placeholder logic, and added anerror
prop which is applied to the TextArea and Input subcomponents to utilize the existing logic they have to make input borders red when there is an input error🚨 User Impact 🚨
Just visual changes to the connector page, but with all of the same functionality kept the same.