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

🪟🧪 Remove empty state page when there are no connectors #19112

Merged
merged 5 commits into from
Nov 29, 2022

Conversation

letiescanciano
Copy link
Contributor

@letiescanciano letiescanciano commented Nov 8, 2022

🎬 Demo: https://www.loom.com/share/6c2860ec8f0a4d95b80eb024f848bdf6 (ignore connection step as we want to keep it as it is)
📓 Experiment details: https://airtable.com/appIuY0uKPVnk8TWT/tbl2SxXnUwf6fVCWS/viww5hSnXY1UUWb6w/rech044wPzAcSNZxV?blocks=hide

❓ Hypothesis
By reducing a step in the setup (source/destination) users will be more likely to interact with the list of connectors.

🎯 Goal
+30% in Airbyte.UI.Source.Select--> Bring users closer to available connectors

What

  • Redirect to set up pages instead of showing empty state

@letiescanciano letiescanciano requested a review from a team as a code owner November 8, 2022 09:57
@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp team/growth labels Nov 8, 2022
@letiescanciano letiescanciano marked this pull request as draft November 8, 2022 14:23
@letiescanciano letiescanciano force-pushed the leti/remove-empty-state-in-connector-setup branch 2 times, most recently from 625c57e to 86832b0 Compare November 14, 2022 08:26
@timroes timroes self-requested a review November 14, 2022 12:15
@letiescanciano letiescanciano force-pushed the leti/remove-empty-state-in-connector-setup branch from 86832b0 to 2b3dc4f Compare November 14, 2022 14:29
@letiescanciano letiescanciano marked this pull request as ready for review November 14, 2022 15:08
@letiescanciano letiescanciano force-pushed the leti/remove-empty-state-in-connector-setup branch 2 times, most recently from a1d760a to 9f50dd1 Compare November 16, 2022 07:37
};

export const openNewDestinationForm = () => {
cy.get(newDestination).click();
cy.wait("@getDestinationsList").then(({ response }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we won’t have a button in the empty state (no destinations set up yet) we need to make a decision based on the response on this request.

@letiescanciano letiescanciano force-pushed the leti/remove-empty-state-in-connector-setup branch from 9f50dd1 to 018a069 Compare November 16, 2022 07:38
};

export const openSourceDestinationFromGrid = (value: string) => {
cy.get("div").contains(value).click();
};

export const openNewSourceForm = () => {
cy.get(newSource).click();
cy.wait("@getSourcesList").then(({ response }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we won’t have a button in the empty state (no sources set up yet) we need to make a decision based on the response on this request.

{title}
</H5>
<H5 className={classNames(styles.title)}>{title}</H5>
{description ? (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding description of what a Source/Destination is in a tooltip since we are loosing the message coming from the empty state

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
{description ? (
{description && (

cy.url().should("include", `/destination/new-destination`);
};

export const openAddSource = () => {
export const aopenAddSource = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems this method got accidentally renamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the issue with the e2e tests failing 🤦‍♀️

import styles from "./Card.module.scss";

export interface CardProps {
title?: React.ReactNode;
description?: React.ReactNode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that this is a basic UI component, I'm not sure we want to add something that specific as the description which renders as an Info icon here, or rather want to stay more generic, and e.g. just allow passing in headerComponents that would render there. @edmundito what are your thoughts around this?

@timroes
Copy link
Collaborator

timroes commented Nov 16, 2022

It seems this is async with master and the e2e tests are still failing. I suspect this will need a bit more cleanup.

@letiescanciano letiescanciano force-pushed the leti/remove-empty-state-in-connector-setup branch from 018a069 to 516a2d3 Compare November 17, 2022 08:02
@letiescanciano
Copy link
Contributor Author

@timroes issue was that function renamed by mistake. CI is passing again now :)

@timroes
Copy link
Collaborator

timroes commented Nov 21, 2022

Just a quick reminder, that EmptyResourceListView (and corresponding code) needs to still be removed, before the final review.

@letiescanciano
Copy link
Contributor Author

Just a quick reminder, that EmptyResourceListView (and corresponding code) needs to still be removed, before the final review.

Thanks! Still have to work on that, but will probably pushed until next week.

* master: (74 commits)
  Fix support icon in sidebar
  fix: add BuildPulse report for helm ac tests (#19785)
  fix: yaml syntax (#19775)
  🪟 🐛 Fix custom connection creation endpoint (#19702)
  Source facebook marketing: check "breakdowns" combinations (#19645)
  fix typo: notify instead of sync (#19737)
  find_non_rate_limited_PAT (#19736)
  Source Google Ads: fix schema for "campaigns" stream (#19700)
  🎉 Source Asana: migrate to new SAT, added base HTTP errors handling (#19561)
  fix order not to randomly fail backward compatibility check (#19377)
  Bump helm chart version reference to 0.42.0 (#19706)
  fix: add extraEnv block (#19703)
  Bump Airbyte version from 0.40.21 to 0.40.22 (#19687)
  Bump helm chart version reference to 0.41.3 (#19685)
  Add connector builder server to airbyte proxy, kube overlays, and helm charts (#19554)
  dbt Cloud integration doc (#19619)
  🪟 🎉 Display service token validation errors in the UI (#19578)
  17644 Update Destination data type test to use the new data types (#19281)
  Docs: fix broken connector builder UI docs links (#19631)
  Bump Airbyte version from 0.40.20 to 0.40.21 (#19634)
  ...
@letiescanciano letiescanciano force-pushed the leti/remove-empty-state-in-connector-setup branch from 31c0fed to 9dc850a Compare November 28, 2022 09:21
)}
<img
className={styles.octavia}
src={`/images/octavia/empty-${resourceType}.png`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those images in /public/images/octavia/empty-... should still be removed, since they are not used anywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right. Removed!

Copy link
Collaborator

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Code LGTM. Checked locally, navigating to those pages seems to work all as expected.

Copy link
Collaborator

@timroes timroes left a comment

Choose a reason for hiding this comment

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

The refactoring on the Card actually broke the layout in the settings pages (buttons now on the left side):

screenshot-20221128-231430

Copy link
Contributor Author

@letiescanciano letiescanciano left a comment

Choose a reason for hiding this comment

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

Fixed! thanks for the double check ;)
Screen Shot 2022-11-29 at 10 16 25 AM
Screen Shot 2022-11-29 at 10 16 30 AM

Copy link
Collaborator

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested locally.

@letiescanciano letiescanciano merged commit 8eac44c into master Nov 29, 2022
@letiescanciano letiescanciano deleted the leti/remove-empty-state-in-connector-setup branch November 29, 2022 15:04
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 team/growth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants