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.

Sign up for GitHub

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

🪟 🧹 Splits the imageBlock component in two. #17479

Merged
merged 14 commits into from
Oct 21, 2022
Merged

🪟 🧹 Splits the imageBlock component in two. #17479

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8a3aeae
Splits the imageBlock component.
Sep 30, 2022
d5f9e12
Adds and corrects some styles.
Oct 7, 2022
a3212a4
Finishes cleaning up styles and properties.
Oct 7, 2022
38a2008
Merge branch 'master' into nataly-split-imageBlock
natalyjazzviolin Oct 7, 2022
6a00977
Merge branch 'master' into nataly-split-imageBlock
natalyjazzviolin Oct 11, 2022
4c75d4b
Cleanup.
Oct 11, 2022
fc4e0f8
Merge branch 'master' into nataly-split-imageBlock
natalyjazzviolin Oct 11, 2022
4dfa205
Requested changes 1/2.
Oct 13, 2022
0287eaa
Requestion changes 2/x, margin not working in className.
Oct 13, 2022
864c3f1
Corrects assing className as prop.
Oct 17, 2022
187280d
Merge branch 'master' into nataly-split-imageBlock
natalyjazzviolin Oct 17, 2022
bdb9285
Requested changes.
Oct 19, 2022
97628ee
Merge branch 'master' into nataly-split-imageBlock
natalyjazzviolin Oct 19, 2022
da207f6
Merge branch 'master' into nataly-split-imageBlock
natalyjazzviolin Oct 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions airbyte-webapp/src/components/EntityTable/components/ConnectEntitiesCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from "react";
import { FormattedMessage } from "react-intl";
import styled from "styled-components";

import { ImageBlock } from "components/ui/ImageBlock";
import { NumberBadge } from "components/ui/NumberBadge";

interface IProps {
values: Array<{
Expand All @@ -19,7 +19,7 @@ const Content = styled.div<{ enabled?: boolean }>`
color: ${({ theme, enabled }) => (!enabled ? theme.greyColor40 : "inheret")};
`;

const Image = styled(ImageBlock)`
const Image = styled(NumberBadge)`
margin-right: 6px;
`;

Expand All @@ -31,6 +31,7 @@ const Connector = styled.div`
`;

const ConnectEntitiesCell: React.FC<IProps> = ({ values, enabled, entity }) => {
console.log(values.length);
natalyjazzviolin marked this conversation as resolved.
Show resolved Hide resolved
if (values.length === 1) {
return (
<Content enabled={enabled}>
Expand All @@ -44,7 +45,15 @@ const ConnectEntitiesCell: React.FC<IProps> = ({ values, enabled, entity }) => {
}

if (!values.length) {
return null;
console.log(`No values:`, values.length);
natalyjazzviolin marked this conversation as resolved.
Show resolved Hide resolved
return (
<Content enabled={enabled}>
<Image num={Number(0)} />
<div>
<p>No connections.</p>
</div>
</Content>
);
}

return (
Expand Down
48 changes: 48 additions & 0 deletions airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
@use "../../../scss/colors";
@use "../../../scss/fonts";

.circle {
Copy link
Collaborator

@timroes timroes Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Currently the number in the circle is not really centered:

screenshot-20221012-120219

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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;

height: 20px;
width: 20px;
min-width: 20px;
Copy link
Collaborator

@timroes timroes Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

border-radius: 50%;
text-align: center;
margin-right: 6px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

overflow: hidden;

&.darkBlue {
background: colors.$dark-blue-100;
}

&.green {
background: colors.$green;
color: colors.$white;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}

&.red {
background: colors.$red;
color: colors.$white;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL! That's such a cool website!

}

&.blue {
background: colors.$blue;
color: colors.$white;
}
}

.small {
border-radius: 0%;
}

.number {
font-family: fonts.$primary;
font-style: normal;
font-weight: 600;
font-size: 10px;
color: colors.$white;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

padding: 3px 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


&.light {
font-weight: 500;
}
}
18 changes: 18 additions & 0 deletions airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { ComponentStory, ComponentMeta } from "@storybook/react";

import { NumberBadge } from "./NumberBadge";

export default {
title: "UI/NumberBadge",
component: NumberBadge,
argTypes: {},
} as ComponentMeta<typeof NumberBadge>;

const Template: ComponentStory<typeof NumberBadge> = (args) => <NumberBadge {...args} />;

export const Primary = Template.bind({});
Primary.args = {
img: undefined,
num: undefined,
small: undefined,
};
33 changes: 33 additions & 0 deletions airbyte-webapp/src/components/ui/NumberBadge/NumberBadge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import classnames from "classnames";
import React from "react";

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

interface NumberBadgeProps {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

img?: string;
num?: number;
small?: boolean;
color?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

light?: boolean;
ariaLabel?: string;
}

export const NumberBadge: React.FC<NumberBadgeProps> = ({ num, small, color, light, ariaLabel }) => {
const imageCircleClassnames = classnames({
[styles.circle]: num,
[styles.small]: small && !num,
[styles.darkBlue]: !small && num && !color,
[styles.green]: color === "green",
[styles.red]: color === "red",
[styles.blue]: color === "blue",
[styles.light]: light,
});

const numberStyles = classnames(styles.number, { [styles.light]: light });

return (
<div className={imageCircleClassnames} aria-label={ariaLabel}>
<div className={numberStyles}>{num}</div>
</div>
);
};
1 change: 1 addition & 0 deletions airbyte-webapp/src/components/ui/NumberBadge/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./NumberBadge";