Clean up of dbt cloud prototype UI code #18894

ambirdsall · 2022-11-03T07:17:02Z

What

Closes https://github.com/airbytehq/airbyte-cloud/issues/3186.

js code style changes

remove unused var in executionUrl match destructuring
extract isDbtWebhookConfig predicate
remove final slash from dbt Cloud executionUrl regex
change yup import syntax in transformation tab

scss code style changes

use spacing variables in settings page scss
use spacing variables in dbt transformation tab scss
target octavia image via class, not parent
target dbt logo image via class, not parent

tsx improvements

don’t render <Formik /> unless hasDbtIntegration
use top-level, element-specific class for img in dbt transformation tab scss
merge emptyListContent styles into jobListContainer
s/<p>/<Text>/g
add aria-label to “remove dbt job” X button
remove unused DbtCloudSignupBanner component

text content

conditionalize “the following jobs” text content based on whether dbt jobs exist
render all text content in transformation tab via <FormattedMessage />
rephrase “integration not setup” text

images

use the svg version of the dbt logo
move {🐙,dbt logo} img file to dbt transform tab component directory
remove alt text from {🐙,dbt logo} images

🚨 User Impact 🚨

None; the only differences in app behavior are slight changes to text content and a few cases where a formerly-invisible component is now not rendered at all.

ambirdsall · 2022-11-03T18:01:27Z

...ionPage/pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.tsx

+        <Text className={styles.contextExplanation}>
+          <FormattedMessage id="connection.dbtCloudJobs.explanation" />
+        </Text>
+        {jobs.map((_j, i) => (


Removing the unused _j binding was mentioned in review, but since this is a function argument list, it's a syntax error to have no binding in that position.

Sorry oversaw that. Could we instead rename it just job in this case? Since we don't use the _ notation for those, and that way it would be a better descriptive (not used) variable.

I think @typescript-eslint/no-unused-vars will complain then . If we need a convention for unused vars, isn't _ the best choice?

Yep, the underscore is needed as a cue for the linter. While my default preference was to use a name which communicates the value, that's superfluous here (by jobs.map, a reader ought to have a good sense of what's going on 😄) and a quick grep (I ran rg '\b_\b' in airbyte-webapp/) shows that using _ for unused arguments is already a somewhat-established convention. It helps that we only import lodash functions individually, so there's no risk of shadowing needed utility functions.

Sorry did not notice that we have that checked via TS and not eslint. We could (not as part of this PR!) maybe disable the TS check and replace it by the eslint rule, which actually allows to configure an after-used so it's not failing on parameters that need to be there because we use the ones after it still.

timroes · 2022-11-04T23:46:13Z

...ionPage/pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.tsx

                </label>
                <Input {...field} type="text" />
              </>
            )}
          </Field>
        </div>
-        <button type="button" className={styles.jobListItemDelete} onClick={removeJob}>
+        <button type="button" className={styles.jobListItemDelete} onClick={removeJob} aria-label="Delete job">


The aria-label should be i18ned as well.

I could be wrong - but I think we can use <Button type="clear"> for this sort of transparent/unstyled button?

We can indeed. It isn't quite as good a match for the designs (the main issue is that it's slightly smaller even at size="lg") and adds a pointless div around the button contents, but it's more consistent with the rest of the app and adds small improvements like higher contrast against the background and a subtle darkening of the icon on hover.

josephkmh

Just a few minor comments!

also a nitpick: could we export DbtCloudTransformationsCard from an index.ts barrel file so we don't have a long import like this:

// ConnectionTransformationTab.tsx
import { DbtCloudTransformationsCard } from "./ConnectionTransformationTab/DbtCloudTransformationsCard";

...pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.module.scss

josephkmh · 2022-11-07T10:55:06Z

...ionPage/pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.tsx

+        <Text className={styles.contextExplanation}>
+          <FormattedMessage id="connection.dbtCloudJobs.explanation" />
+        </Text>
+        {jobs.map((_j, i) => (


I think @typescript-eslint/no-unused-vars will complain then . If we need a convention for unused vars, isn't _ the best choice?

...ionPage/pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.tsx

josephkmh · 2022-11-07T11:18:49Z

...ionPage/pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.tsx

                </label>
                <Input {...field} type="text" />
              </>
            )}
          </Field>
        </div>
-        <button type="button" className={styles.jobListItemDelete} onClick={removeJob}>
+        <button type="button" className={styles.jobListItemDelete} onClick={removeJob} aria-label="Delete job">


I could be wrong - but I think we can use <Button type="clear"> for this sort of transparent/unstyled button?

...ionPage/pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.tsx

- i18nize the aria-label - replace most custom scss by using `<Button variant="clear" size="lg">`

josephkmh

LGTM 🚀

* Clean up of dbt cloud prototype UI code * Remove variables->vars alias from scss files * Remove obsolete comments * Improvements to delete job button - i18nize the aria-label - replace most custom scss by using `<Button variant="clear" size="lg">` * Use `_` for unused function arg

ambirdsall requested a review from a team as a code owner November 3, 2022 07:17

github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 3, 2022

ambirdsall requested review from timroes and josephkmh November 3, 2022 17:57

ambirdsall commented Nov 3, 2022

View reviewed changes

ambirdsall force-pushed the alex/dbt-cloud-demo-code-cleanup branch from 4d8f042 to 7b8d233 Compare November 4, 2022 18:55

timroes reviewed Nov 4, 2022

View reviewed changes

timroes mentioned this pull request Nov 5, 2022

🪟 🧹 Cleanup alt attributes on images #19000

Merged

josephkmh requested changes Nov 7, 2022

View reviewed changes

ambirdsall force-pushed the alex/dbt-cloud-demo-code-cleanup branch from 7b8d233 to 6e8c2d0 Compare November 9, 2022 00:47

ambirdsall requested a review from josephkmh November 9, 2022 01:03

ambirdsall added 5 commits November 8, 2022 17:04

Clean up of dbt cloud prototype UI code

4de404b

Remove variables->vars alias from scss files

2b802fe

Remove obsolete comments

1dfc1a9

Improvements to delete job button

15db1c3

- i18nize the aria-label - replace most custom scss by using `<Button variant="clear" size="lg">`

Use _ for unused function arg

c349a1a

ambirdsall force-pushed the alex/dbt-cloud-demo-code-cleanup branch from 8119dc2 to c349a1a Compare November 9, 2022 01:04

josephkmh approved these changes Nov 9, 2022

View reviewed changes

ambirdsall merged commit 4a577e9 into master Nov 10, 2022

ambirdsall deleted the alex/dbt-cloud-demo-code-cleanup branch November 10, 2022 00:27

github-actions bot mentioned this pull request Nov 15, 2022

Bump helm chart version reference to 0.40.43 #19453

Closed

octavia-squidington-iii mentioned this pull request Nov 18, 2022

Bump Airbyte version from 0.40.18 to 0.40.19 #19579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up of dbt cloud prototype UI code #18894

Clean up of dbt cloud prototype UI code #18894

ambirdsall commented Nov 3, 2022 •

edited

Loading

ambirdsall Nov 3, 2022

timroes Nov 4, 2022

josephkmh Nov 7, 2022

ambirdsall Nov 8, 2022

timroes Nov 8, 2022

timroes Nov 4, 2022

josephkmh Nov 7, 2022

ambirdsall Nov 9, 2022

josephkmh left a comment

josephkmh Nov 7, 2022

josephkmh Nov 7, 2022

josephkmh left a comment

Clean up of dbt cloud prototype UI code #18894

Clean up of dbt cloud prototype UI code #18894

Conversation

ambirdsall commented Nov 3, 2022 • edited Loading

What

js code style changes

scss code style changes

tsx improvements

text content

images

🚨 User Impact 🚨

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephkmh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephkmh left a comment

Choose a reason for hiding this comment

ambirdsall commented Nov 3, 2022 •

edited

Loading