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

Move tooltip to next #887

Merged
merged 6 commits into from
Mar 15, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
22 changes: 11 additions & 11 deletions docs/CODE_GUIDELINES.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ Here's another example:

### CSS Nesting

EDS uses [PostCSS Nested](https://github.com/postcss/postcss-nested) to provide some developer ergononics. As a general principle, nesting should be used sparingly and is only used in the following situations:
EDS uses [PostCSS Nested](https://github.com/postcss/postcss-nested) to provide some developer ergonomics. As a general principle, nesting should be used sparingly and is only used in the following situations:
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow thanks for all the spelling updates! ❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

I somehow feel like this was some sort of test and you passed.


- Media queries
- States and pseudo-selectors
Expand Down Expand Up @@ -127,7 +127,7 @@ EDS uses [PostCSS Nested](https://github.com/postcss/postcss-nested) to provide

#### Parent selectors

Use [parent selectors](https://sass-lang.com/documentation/style-rules/parent-selector) to target a selector when it appears inside a specific parent element. Use parent selectors instead of child selectors in order to co-locate all styles around a specific selector, which improves maintability and findability.
Use [parent selectors](https://sass-lang.com/documentation/style-rules/parent-selector) to target a selector when it appears inside a specific parent element. Use parent selectors instead of child selectors in order to co-locate all styles around a specific selector, which improves maintainability and findability.

Use the following conventions:

Expand Down Expand Up @@ -188,7 +188,7 @@ For example:
}
```

Not all CSS declarations warrant a comment, but non-obvious declarations (like context-specific styles, magic numbers, or styles dependant on other selector styles) should be accompanied by a comment.
Not all CSS declarations warrant a comment, but non-obvious declarations (like context-specific styles, magic numbers, or styles dependent on other selector styles) should be accompanied by a comment.

### Other CSS Rules

Expand Down Expand Up @@ -261,7 +261,7 @@ Please refer to the [design tokens documentation](./TOKENS.md) to learn how to u

- **Presentational Components Only** - EDS provides a library of reusable [presentational UI components](https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0) that are consumed by CZI applications. These presentational components are "dumb" and don't contain any application business logic and aren't hooked up to any data models.
- **Predictable APIs** - EDS provides consistent, clear [component APIs](#component-naming) in order to provide a consistent and intuitive user developer experience.
- **Composition over inheritence** EDS adheres to the [composition over inheritence](https://en.wikipedia.org/wiki/Composition_over_inheritance) principle in order to create clean, extensible components that aren't tied to specific contexts or content.
- **Composition over inheritance** EDS adheres to the [composition over inheritance](https://en.wikipedia.org/wiki/Composition_over_inheritance) principle in order to create clean, extensible components that aren't tied to specific contexts or content.

## JavaScript Tools <a name="js-tools"></a>

Expand Down Expand Up @@ -320,7 +320,7 @@ export interface Props {
}
```

All component props must be defined with appropriate [TypeScript type](https://www.typescriptlang.org/docs/handbook/basic-types.html) applied. Each prop must contain a comment above the prop declaration to document the prop's function. These comments and prop declarations are automatically converted into prop documentation in Storybook. As a general guideline, try to organize component prop definitiones alphabetically.
All component props must be defined with appropriate [TypeScript type](https://www.typescriptlang.org/docs/handbook/basic-types.html) applied. Each prop must contain a comment above the prop declaration to document the prop's function. These comments and prop declarations are automatically converted into prop documentation in Storybook. As a general guideline, try to organize component prop definitions alphabetically.

### Export module

Expand All @@ -333,7 +333,7 @@ export const ComponentName: React.FC<Props> = ({
}) => {
```

This defines the component name and passses in all the `Props`.
This defines the component name and passes in all the `Props`.

### Variables, Methods, and Hooks (if applicable)

Expand Down Expand Up @@ -371,7 +371,7 @@ Components in this library exist in a flat structure so each component directory

Certain components (such as `<Tabs />` and `<Table />`) require splitting up into smaller subcomponents.

By default, we err towards more centralized control over the component architecture in order to prevent undesired results (for instance, we don't want users to put a `<Card />` inside of `<Breadcrumbs />`). However, certain components will require more flexibility and will therefore be architected to be composible and flexible (such as `<Card>`).
By default, we err towards more centralized control over the component architecture in order to prevent undesired results (for instance, we don't want users to put a `<Card />` inside of `<Breadcrumbs />`). However, certain components will require more flexibility and will therefore be architected to be composable and flexible (such as `<Card>`).

#### Conventions for compound components

Expand All @@ -391,17 +391,17 @@ By default, we err towards more centralized control over the component architect
EDS follows specific front-end API naming conventions. Authoring a consistent API language provides many benefits:

- **More efficient development** - Because the API language is consistent across components, user developers can spend more time coding rather than reading API documentation. Also, library contributors don't have to think as much about component API naming when creating new components/variants.
- **Shared vocuabulary between designers and developers** - When the code library and design library use the same language, designers and developers can spend more time collaborating rather than futzing over what things are named. This improves team velocity and product quality. It also positions the team to benefit from future tooling that can bring design and code closer together (something many startups and plugins are trying to solve right now!)
- **Shared vocabulary between designers and developers** - When the code library and design library use the same language, designers and developers can spend more time collaborating rather than futzing over what things are named. This improves team velocity and product quality. It also positions the team to benefit from future tooling that can bring design and code closer together (something many startups and plugins are trying to solve right now!)
- **Future changes** - Utilizing a consistent language means that future changes and improvements are as easy as find-and-replace.

EDS adhreres to the following API naming conventions:
EDS adheres to the following API naming conventions:

### Variants

- `variant` should be used for primary _stylistic_ variations of a component, such as (e.g. `<Card variant="bordered">` or `<Button variant="secondary">`). `variant` should be used if there is primarily one variable used to manipulate the component style.
- `inverted` should be used consistently for stylistic `variation`s that "invert" the color schemes (e.g. `inverted=true`) to work on a darker background.
- `size` should be used for adjusting size attributes (e.g. `<Button variant="secondary" size="sm">` or `<Button size="lg"`>). Default to `sm` and `lg`, with "md" being the default.
- `behavior` should be used for funcitonal variations of a pattern, such as `<Banner behavior="dismissable">`. Additional non-exclusive behaviors should be handled using boolean props prefixed with `is` (e.g. `isSticky` and `isDismissable`).
- `behavior` should be used for functional variations of a pattern, such as `<Banner behavior="dismissable">`. Additional non-exclusive behaviors should be handled using boolean props prefixed with `is` (e.g. `isSticky` and `isDismissable`).
- `orientation` should be used for controlling the layout or orientation of a component (e.g. `<ButtonGroup orientation="stacked">`)
- `align` should be used for aligning content, and should include `left` (default), `center`, `right` if needed.
- `verticalAlign` should be used for vertically aligning content, and should include `top`, `middle`, `bottom` if needed.
Expand All @@ -410,7 +410,7 @@ EDS adhreres to the following API naming conventions:

- Default to `text` for strings of text, such as `<Button text="First Name">`.
- For headings, default to `title`, such as `<PageHeader title="My Page Title">`.
- For form-related comnp, use the semantic `label` or `legend` (e.g. `<TextField label="first name" />`).
- For form-related component, use the semantic `label` or `legend` (e.g. `<TextField label="first name" />`).

### Tag name

Expand Down
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
"react": ">= 16.8.0"
},
"dependencies": {
"@tippyjs/react": "4.2.5",
"clsx": "^1.1.1",
"downshift": "^6.1.7",
"nanoid": "^3.3.1",
Expand All @@ -70,6 +71,8 @@
"@storybook/addon-postcss": "^2.0.0",
"@storybook/addon-storyshots": "^6.4.19",
"@storybook/react": "^6.4.19",
"@storybook/testing-library": "^0.0.9",
"@storybook/testing-react": "^1.2.3",
"@testing-library/jest-dom": "^5.16.2",
"@testing-library/react": "^12.1.3",
"@testing-library/user-event": "^13.5.0",
Expand Down
18 changes: 16 additions & 2 deletions src/components/Counter/Counter.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Story, Meta } from '@storybook/react';
import React from 'react';
import { Counter, Props } from './Counter';
import { Button } from '../Button/Button';
import { Tooltip } from '../Tooltip/Tooltip';

export default {
Expand All @@ -20,6 +21,19 @@ Default.args = {
plusButtonText: 'Add by 1',
};

/* TODO: replace <ForwardRefButton> with EDS button since it's already using forwardRef */
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

could we quickly add forwardRef to the existing button now? not sure how long EDS button conversion will take & it's nicer not having to hunt down TODOs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep we can, and will do

const ForwardRefButton = React.forwardRef((props, ref) => (
<Button
variant="bare"
iconPosition="after"
iconName="question-mark-circle"
aria-label="Hover this button to trigger the tooltip"
text="Hover this button to trigger the tooltip"
buttonRef={ref}
/>
));
ForwardRefButton.displayName = 'ForwardRefButton';

export const DefaultWithTooltip = Template.bind({});
DefaultWithTooltip.args = {
value: 1,
Expand All @@ -30,8 +44,8 @@ DefaultWithTooltip.args = {
plusButtonText: 'Add by 1',
fieldNote: 'This is a counter field',
labelAfter: (
<Tooltip buttonText="Select this button to trigger the tooltip">
Some text to help with a form field
<Tooltip content="Some text to help with a form field">
<ForwardRefButton />
</Tooltip>
),
};
Expand Down
152 changes: 103 additions & 49 deletions src/components/Tooltip/Tooltip.module.css
100755 → 100644
Original file line number Diff line number Diff line change
@@ -1,72 +1,126 @@
@import '../../design-tokens/mixins.css';
/* stylelint-disable selector-pseudo-class-no-unknown */
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add to stylelintrc


/*------------------------------------*\
#TOOLTIP
\*------------------------------------*/

/**
* 1) An icon that appears
* 1) Tippyjs provides .tippy-box, which has two child elements: .tippy-content and .tippy-arrow
* 2) Tippyjs also attaches data-attribute `data-placement` depending on how the tooltip is aligned
*/

.tooltip {
display: inline-block;
position: relative;
overflow: visible;
border-style: solid;
border-width: var(--eds-border-width-sm);
border-radius: var(--eds-border-radius-lg);
box-shadow: var(--eds-theme-box-shadow);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this shadow is thinner than the original shadow, but using what is available for consistency
original on the left, this pr on the right:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need a ticket for updating the tokens 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I think all tokens are subject to change while design is doing discovery right now; so we can feel free to replace the values with our existing ones

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, to clarify, are you saying to add a token to shadows.json, or not using a token for now and assigning a value to box-shadow?

Copy link
Contributor

@anniehu4 anniehu4 Mar 8, 2022

Choose a reason for hiding this comment

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

either add a token to shadows.json or update one of the base tokens to match ours 🙂 we should always be using a tier 2 token, but the values are owned by us, and will probably evolve over the next few weeks. so not sure it's worth to update all of them up front, but if we notice discrepancies we can default to the values we were already using for the component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks for the explanation, will update shadow token md,

Copy link
Contributor

Choose a reason for hiding this comment

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

@anniehu4 @jinlee93 @dierat yep the token work is firming up, and I'm going to take the lead on an initial import of what they've put in place in Figma. Hoping to get that in there by the end of the day.


@media (prefers-reduced-motion) {
transition-property: none;
}
}

.tooltip__trigger {
display: inline-block;
cursor: pointer;
/**
* 1) Enables opacity fade animation
*/
.tooltip[data-state="hidden"] {
opacity: 0;
}

&:focus-visible {
@mixin focus;
}
.tooltip :global(.tippy-content) {
padding-left: var(--eds-size-2);
padding-right: var(--eds-size-2);
padding-bottom: var(--eds-size-1);
padding-top: var(--eds-size-1);
}

/**
* 1) Color Variants
*/
.tooltip--light {
border-color: var(--eds-color-neutral-300);
color: var(--eds-color-neutral-700);
background-color: var(--eds-color-neutral-100);
--arrow-color: var(--eds-color-neutral-300);
}

.tooltip--dark {
border-color: var(--eds-color-neutral-700);
color: var(--eds-color-neutral-white);
background-color: var(--eds-color-neutral-700);
--arrow-color: var(--eds-color-neutral-700);
}

.tooltip__content {
display: none;
/**
* 1) Add arrows
*/
.tooltip :global(.tippy-arrow) {
position: absolute;
bottom: 130%;
left: 50%;
margin-left: -96px;
width: 12rem;
padding: var(--eds-size-1);
background: var(--eds-color-neutral-white);
color: var(--eds-theme-color-body-foreground);
border-width: var(--eds-theme-border-width);

width: var(--eds-size-2);
height: var(--eds-size-2);
}

.tooltip :global(.tippy-arrow::before) {
position: absolute;

border-style: solid;
border-color: var(--eds-theme-color-neutral-subtle-border);
border-radius: var(--eds-theme-border-radius);
box-shadow: var(--eds-theme-box-shadow);
border-color: transparent;
border-width: var(--eds-size-1);

&:focus-visible {
@mixin focus;
}
content: "";
}

.tooltip--right & {
bottom: auto;
top: -50%;
left: 130%;
margin-left: 0;
}
.tooltip[data-placement^="top"] :global(.tippy-arrow) {
left: 0;

.tooltip--below & {
bottom: inherit;
}
transform: translate3d(73px, 0, 0);
}

.tooltip--left & {
bottom: auto;
left: auto;
top: -50%;
right: calc(130% + 5px);
}
.tooltip[data-placement^="bottom"] :global(.tippy-arrow) {
top: 0;
left: 0;

.tooltip.eds-is-active & {
display: flex;
justify-content: center;
z-index: var(--eds-z-index-100);
}
transform: translate3d(73px, 0, 0);
}

.tooltip[data-placement^="left"] :global(.tippy-arrow) {
top: 0;
right: 0;

transform: translate3d(0, 19px, 0);
}

.tooltip[data-placement^="right"] :global(.tippy-arrow) {
top: 0;
left: 0;

transform: translate3d(0, 25px, 0);
}

.tooltip[data-placement^="top"] :global(.tippy-arrow::before) {
left: 0;

border-top-color: var(--arrow-color);
border-bottom-width: 0;
}

.tooltip[data-placement^="bottom"] :global(.tippy-arrow::before) {
left: 0;

border-bottom-color: var(--arrow-color);
border-top-width: 0;
top: -7px;
}

.tooltip[data-placement^="left"] :global(.tippy-arrow::before) {
border-left-color: var(--arrow-color);
border-right-width: 0;
right: -7px;
}

.tooltip__icon {
fill: var(--eds-theme-color-primary-foreground);
.tooltip[data-placement^="right"] :global(.tippy-arrow::before) {
border-right-color: var(--arrow-color);
border-left-width: 0;
left: -7px;
}
29 changes: 29 additions & 0 deletions src/components/Tooltip/Tooltip.stories.module.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*------------------------------------*\
#TOOLTIP STORIES
\*------------------------------------*/

/**
* 1) Spacing is required for the Tooltip to be placed where alignment is indicated,
* otherwise Tippy will place somewhere else due to lack of space
*/

.trigger--spacing {
margin: 9rem;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird number but it's appropriate required for the tooltip to place where alignment is indicated, otherwise Tippy places somewhere else due to lack of space

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe leave that in a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, added comment

Copy link
Contributor

Choose a reason for hiding this comment

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

let's also do the footnote commenting thing brad had, i.e.:

Suggested change
margin: 9rem;
margin: 9rem; /* 1 */

to note that this specific line is connected to the 1) comment above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

}

.trigger--spacing-top {
margin-top: 9rem;
}
.trigger--spacing-bottom {
margin-bottom: 9rem;
}
.trigger--spacing-left {
margin-left: 9rem;
}
.trigger--spacing-right {
margin-right: 9rem;
}

.trigger--spacing-left-large {
margin-left: 24rem;
}
Loading