🪟 🧹 Splits the imageBlock component in two. #17479

natalyjazzviolin · 2022-09-30T20:11:54Z

What

Closes Split circled number away from ImageBlock #17073
Splits the ImageBlock component into two, and creates NumberBadge component.

🚨 User Impact 🚨

No breaking changes.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

edmundito

I tested the storybook locally and there were a couple of areas that don't seem to be working with the number badge: The size and light mode

airbyte-webapp/src/components/EntityTable/components/ConnectEntitiesCell.tsx

edmundito · 2022-10-05T09:54:40Z

I just noticed this was a draft. Whoops.

natalyjazzviolin · 2022-10-07T20:22:50Z

I just noticed this was a draft. Whoops.

@edmundito Thank you for the comments nevertheless!

timroes · 2022-10-12T10:01:16Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.tsx

+
+interface NumberBadgeProps {
+  value: number;
+  color?: string;


This should be typed so we know which value we expect:

Suggested change

color?: string;

color?: "green" | "red" | "blue" | "default";

Also since the lack of the value has currently a darkBlue color, we should make sure there's a way to specify that as well via an explicit value (i.e. default in this case), so the logic for the colors below also needs adjustments to handle this.

@timroes I'll rename the darkBlue to default - it's so faded it looks like grey and I think the name is confusing!

timroes

Requested some changes about code and some minor issue. Overall looks good to me

timroes · 2022-10-12T10:02:56Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.stories.tsx

+
+export const Primary = Template.bind({});
+Primary.args = {
+  value: undefined,


We should give this a number by default, so the storybook doesn't just render an empty circle by default.

timroes · 2022-10-12T10:04:08Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.module.scss

+.circle {
+  height: 20px;
+  width: 20px;
+  min-width: 20px;


~~There should be no need for the min-width, since we're already setting the width explicitaly to 20px.~~

Ignore this, instead see my comment above about making this work with longer numbers as well.

timroes · 2022-10-12T10:08:44Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.module.scss

+@use "../../../scss/colors";
+@use "../../../scss/fonts";
+
+.circle {


Currently the number in the circle is not really centered:

We can solve this by adding:

display: flex; justify-content: center; align-items: center; line-height: normal;

to the .circle class here. We need the line-height because we're using this component in some cases where the parent sets a custom line-height, which would mess up the vertical centering.

In addition I'd suggest we try making this component also work if there are longer numbers in it, which would require the following changes:

width: fit-content; min-width: 20px; border-radius: 10px; padding: 0 4px;

timroes · 2022-10-12T10:10:25Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.module.scss

+  font-weight: 500;
+  font-size: 10px;
+  color: colors.$white;
+  padding: 3px 0;


With the changes mentioned above to center the number, the padding isn't having any effect anymore and should be removed.

timroes · 2022-10-12T10:26:20Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.module.scss

+  min-width: 20px;
+  border-radius: 50%;
+  text-align: center;
+  margin-right: 6px;


I don't think we want the margin-right on this component. This is meant in a place this is used to keep distance from another element. The place where this is used should apply this and not the component itself, since it does not really have any idea about the actual surroundings where this is used.

The ConnectEntitiesCell already sets a margin on this component anyway, in the DiffIconBlock we can increase the gap property on the parent flex box container to give more spacing between the numbers.

timroes · 2022-10-12T10:27:32Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.tsx

+
+import styles from "./NumberBadge.module.scss";
+
+interface NumberBadgeProps {


We should accept className as a property on all components and merge it with the other classnames for the wrapping div. Otherwise restyling, e.g. adding margin to this component in consuming places won't work.

timroes · 2022-10-18T14:02:31Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.tsx

+
+export const NumberBadge: React.FC<NumberBadgeProps> = ({ value, color, className, "aria-label": ariaLabel }) => {
+  const numberBadgeClassnames = classnames(styles.circle, className, {
+    [styles.default]: !color,


Suggested change

[styles.default]: !color,

[styles.default]: !color || color === "default",

So that the style will also apply when a user explicitaly set default as a property value.

timroes · 2022-10-19T09:55:27Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.module.scss

+  font-style: normal;
+  font-weight: 500;
+  font-size: 10px;
+  color: colors.$white;


This should be removed, so the text colors from the above .circle.red etc are actually having an affect.

timroes · 2022-10-19T09:56:54Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.module.scss

+  font-family: fonts.$primary;
+  font-style: normal;
+  font-weight: 500;
+  font-size: 10px;


All those styles could be moved to the circle class above instead, so we wouldn't need this class at all.

timroes · 2022-10-19T09:59:29Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.module.scss

+
+  &.red {
+    background: colors.$red;
+    color: colors.$white;


This needs to be colors.$black to have proper contrast ratio between text and background:

✔️ https://color.review/check/000000-FF5E7B
❌ https://color.review/check/FFFFFF-FF5E7B

TIL! That's such a cool website!

timroes · 2022-10-19T10:00:37Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.module.scss

+
+  &.green {
+    background: colors.$green;
+    color: colors.$white;


This needs to be colors.$black to have proper contrast ratio between text and background:

✔️ https://color.review/check/000000-67DAE1
❌ https://color.review/check/FFFFFF-67DAE1

timroes · 2022-10-19T10:00:49Z

airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.module.scss

+
+  &.green {
+    background: colors.$green;
+    color: colors.$white;


This needs to be colors.$black to have proper contrast ratio between text and background:

✔️ https://color.review/check/000000-67DAE1
❌ https://color.review/check/FFFFFF-67DAE1

timroes

Tested Story book component and connector listing table locally (Chrome Linux). Works all as expected. Code LGTM

* Splits the imageBlock component. * Adds and corrects some styles. * Finishes cleaning up styles and properties. * Cleanup. * Requested changes 1/2. * Requestion changes 2/x, margin not working in className. * Corrects assing className as prop. * Requested changes.

Splits the imageBlock component.

8a3aeae

natalyjazzviolin requested a review from a team as a code owner September 30, 2022 20:11

natalyjazzviolin marked this pull request as draft September 30, 2022 20:11

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

edmundito suggested changes Oct 5, 2022

View reviewed changes

airbyte-webapp/src/components/EntityTable/components/ConnectEntitiesCell.tsx Outdated Show resolved Hide resolved

airbyte-webapp/src/components/EntityTable/components/ConnectEntitiesCell.tsx Outdated Show resolved Hide resolved

edmundito requested a review from timroes October 5, 2022 09:54

nataly added 2 commits October 7, 2022 10:41

Adds and corrects some styles.

d5f9e12

Finishes cleaning up styles and properties.

a3212a4

natalyjazzviolin added 2 commits October 7, 2022 16:24

Merge branch 'master' into nataly-split-imageBlock

38a2008

Merge branch 'master' into nataly-split-imageBlock

6a00977

natalyjazzviolin marked this pull request as ready for review October 11, 2022 13:49

Cleanup.

4c75d4b

natalyjazzviolin requested a review from edmundito October 11, 2022 14:29

Merge branch 'master' into nataly-split-imageBlock

fc4e0f8

timroes reviewed Oct 12, 2022

View reviewed changes

timroes requested changes Oct 12, 2022

View reviewed changes

timroes reviewed Oct 12, 2022

View reviewed changes

nataly added 2 commits October 13, 2022 09:51

Requested changes 1/2.

4dfa205

Requestion changes 2/x, margin not working in className.

0287eaa

natalyjazzviolin marked this pull request as draft October 14, 2022 21:23

nataly and others added 2 commits October 17, 2022 08:41

Corrects assing className as prop.

864c3f1

Merge branch 'master' into nataly-split-imageBlock

187280d

natalyjazzviolin marked this pull request as ready for review October 17, 2022 12:41

natalyjazzviolin requested a review from timroes October 17, 2022 12:41

timroes reviewed Oct 18, 2022

View reviewed changes

timroes reviewed Oct 19, 2022

View reviewed changes

nataly and others added 2 commits October 19, 2022 10:49

Requested changes.

bdb9285

Merge branch 'master' into nataly-split-imageBlock

97628ee

natalyjazzviolin requested a review from timroes October 19, 2022 16:01

timroes approved these changes Oct 20, 2022

View reviewed changes

Merge branch 'master' into nataly-split-imageBlock

da207f6

natalyjazzviolin merged commit 2e0fae4 into master Oct 21, 2022

natalyjazzviolin deleted the nataly-split-imageBlock branch October 21, 2022 13:39

octavia-squidington-iii mentioned this pull request Oct 21, 2022

Bump Airbyte version from 0.40.16 to 0.40.17 #18316

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🧹 Splits the imageBlock component in two. #17479

🪟 🧹 Splits the imageBlock component in two. #17479

natalyjazzviolin commented Sep 30, 2022

edmundito left a comment

edmundito commented Oct 5, 2022

natalyjazzviolin commented Oct 7, 2022 •

edited

Loading

timroes Oct 12, 2022

natalyjazzviolin Oct 13, 2022

timroes left a comment

timroes Oct 12, 2022

timroes Oct 12, 2022 •

edited

Loading

timroes Oct 12, 2022 •

edited

Loading

timroes Oct 12, 2022

timroes Oct 12, 2022

timroes Oct 12, 2022

timroes Oct 12, 2022

timroes Oct 18, 2022

timroes Oct 18, 2022

timroes Oct 19, 2022

timroes Oct 19, 2022

timroes Oct 19, 2022

natalyjazzviolin Oct 19, 2022

timroes Oct 19, 2022

timroes Oct 19, 2022

timroes left a comment

	color?: string;
	color?: "green" \| "red" \| "blue" \| "default";


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

		interface NumberBadgeProps {

	[styles.default]: !color,
	[styles.default]: !color \|\| color === "default",

🪟 🧹 Splits the imageBlock component in two. #17479

🪟 🧹 Splits the imageBlock component in two. #17479

Conversation

natalyjazzviolin commented Sep 30, 2022

What

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

edmundito left a comment

Choose a reason for hiding this comment

edmundito commented Oct 5, 2022

natalyjazzviolin commented Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

timroes Oct 12, 2022 • edited Loading

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

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

timroes left a comment

Choose a reason for hiding this comment

natalyjazzviolin commented Oct 7, 2022 •

edited

Loading

timroes Oct 12, 2022 •

edited

Loading

timroes Oct 12, 2022 •

edited

Loading