-
Notifications
You must be signed in to change notification settings - Fork 12
ZIndex: base implementation + story #564
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 600efbf The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
import { Meta } from "@storybook/addon-docs"; | ||
import ZIndex from "./ZIndex"; | ||
|
||
<Meta title="z-Index" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autodocs unfortunately dont work here because it's a function and not an actual component. so i manually created docs on how to use this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@somethiiing thanks for putting this together!
I like the overall directions but just had a couple of notes:
- How are we thinking of enforcing this? In Gestalt, we did this by enforcing a certain type e.g.
Indexable
instead ofnumber
. See https://github.com/pinterest/gestalt/blob/d86286b7f74e51e24bf5ce67ac55e6a208205624/packages/gestalt/src/Box.tsx#L518 & https://gestalt.pinterest.systems/web/zindex_classes - Ideally we make the zIndex's explicitly depend on each other. E.g. how do we know whether we should pass a
localLayer
of0
,1
,2
or3
? That usually depends on another component - Be sure to add
getZIndex
to the syntax core exportssyntax/packages/syntax-core/src/index.tsx
Lines 1 to 31 in 804cc97
import Avatar from "./Avatar/Avatar"; import AvatarGroup from "./AvatarGroup/AvatarGroup"; import Badge from "./Badge/Badge"; import Box from "./Box/Box"; import Button from "./Button/Button"; import ButtonGroup from "./ButtonGroup/ButtonGroup"; import Card from "./Card/Card"; import Checkbox from "./Checkbox/Checkbox"; import Chip from "./Chip/Chip"; import Divider from "./Divider/Divider"; import Heading from "./Heading/Heading"; import Icon from "./Icon/Icon"; import IconButton from "./IconButton/IconButton"; import IconLinkButton from "./IconLinkButton/IconLinkButton"; import LinkButton from "./LinkButton/LinkButton"; import Modal from "./Modal/Modal"; import Popover from "./Popover/Popover"; import RadioButton from "./RadioButton/RadioButton"; import RichSelectList from "./RichSelect/RichSelectList"; import SelectList from "./SelectList/SelectList"; import TabButton from "./TabButton/TabButton"; import TabLink from "./TabLink/TabLink"; import Tabs from "./Tabs/Tabs"; import TapArea from "./TapArea/TapArea"; import TextArea from "./TextArea/TextArea"; import TextField from "./TextField/TextField"; import ThemeProvider from "./ThemeProvider/ThemeProvider"; import Tooltip from "./Tooltip/Tooltip"; import Typography from "./Typography/Typography"; import WordConfetti from "./WordConfetti/WordConfetti"; - Just commenting for now since it would be good for someone else to also take a deeper look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this happen Wilson! This is a great start. I haven't looked deeply at your code, but added a few high level thoughts/questions to start.
*/ | ||
localLayer: -3 | -2 | -1 | 0 | 1 | 2 | 3 = 0, | ||
): number { | ||
const layerIndex = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How well do these particular layers correspond to how we are using z-indexes in the app right now? I think it would be helpful to audit where we currently need z-indexes and for what types of components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep in mind that some z-indexes might not even be explicitly set in our code. For example, it seems that the MUI Dialog component has a z-index of 1300 (see previous discussion), and it appears we are still using that in some places in our code.
* | ||
* @defaultValue `0` | ||
*/ | ||
localLayer: -3 | -2 | -1 | 0 | 1 | 2 | 3 = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will folks know which localLayer to choose?
): number { | ||
const layerIndex = { | ||
base: 0, | ||
menu: 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having comments/explanations for each of these layers will be really helpful, so people know which one is appropriate for their use case. Just from the names, I'm not sure when to use "base", "sticky", or "fixed", and when I should use "modal" vs "popup".
* <Modal zIndex={getZIndex("modal")} /> | ||
* ``` | ||
*/ | ||
export default function getZIndex( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are you thinking the migration process would work, to move from our current z-indexes to these? Would we need to migrate all z-indexes at once?
Z-Index implementation based off of Outlayers UI Layer System
Basically has 6 "groups" of layers. and a
localLayer
prop to adjust between the layers if necessarynote: the story is a little gross because of where it's located in the storybook structure, where it's located it doesn't have color tokens so alot of these colors/backgroundcolors/etc are manually set