ItsJonQ / g2

✨ An experimental reimagining of WordPress components
http://g2-components.com/
MIT License
105 stars 12 forks source link

Remove silent fail conditions #297

Closed sarayourfriend closed 3 years ago

sarayourfriend commented 3 years ago

Removes some silent fail conditions that were already covered by the types

Also refines a few type for some functions.

Note: when we upgrade gutenberg to use this version we'll need to remove the ZIndex fallback values for TypeScript to stop complaining about them :)

vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/itsjonq/g2/G8ubQMatmMiPRNDPwQKfgK4PWBr6
✅ Preview: https://g2-git-remove-silent-fail-conditions-itsjonq.vercel.app

ItsJonQ commented 3 years ago

Haiiiiii!!!

The zIndex one is interesting. The original design allowed for the function both get and register new values (if one wasn't defined) - for example, ui.zIndex('LoginDropdown', 100).

I'm not sure how you'd go about typing this. I suppose the other question is... would we want this workflow to begin with.

If not, what would be a good way for folks to register/customize z-index values :).

Note: I think the previous workflow was a start of something. I think it could be improved though.

sarayourfriend commented 3 years ago

I'm not sure how you'd go about typing this.

Unfortunately you cannot. I think this is a sign that this isn't necessarily a good idea.

The original design allowed for the function both get and register new values

I think we should remove the register new values part in favor of manually adding z-indexes to the map of all z-indexes. Otherwise why have the utility at all? In my view the nice thing about the utility is that it forces you to document z-indexes especially in relationship to each other. If you can register them on the fly that defeats the purpose of that at least and deviates from the current SASS function that exists in Gutenberg.

What's the use case where you absolutely need to be able to register a new z-index on the fly?

ItsJonQ commented 3 years ago

What's the use case where you absolutely need to be able to register a new z-index on the fly?

If we're only working within Gutenberg, we have control to define/add zIndexes as needed by adjusting the Z_INDEX_REGISTRY. That part is 👍 .


TLDR (of below):

I think you're right. I'm not quite sure of the optimal workflow for managing z-index within G2 x Gutenberg x WP Admin yet :).


I'm just thinking about block developers or attempting to use ui.zIndex beyond Gutenberg (e.g. WP Admin). More specifically, if folks wanted to define their own series of values.

Taking a step back... managing z-index has always been a challenge (in general).

I think the current agreed upon workflow is similar to what you've suggested:

Define a set -> Register -> Use

After register, there's no way to modify values after the fact.

In the CSS-in-JS + Theme workflow, this is usually done by specifying zIndices (or something similar) in a theme object being passed to a global wrapping ThemeProvider of some kind:

https://chakra-ui.com/docs/theming/theme#z-index-values

sarayourfriend commented 3 years ago

block developers or attempting to use ui.zIndex beyond Gutenberg

This is a good point, I hadn't considered it... If we want to support this then we have to lose the strictness of the zIndex helper's first parameter 🤔 Though I wonder if in in most cases for block developers we could recommend that they a) maintain their own set of z-indexes however they choose and/or b) would base their z-indexes on the ones that exist in the style system using simple math (as indeed we do in some places in Gutenberg today).

ItsJonQ commented 3 years ago

The A and/or B solution sounds good to me :). I think it would be good to provide a workflow for A.

I'm not sure what that would be though 🤔

sarayourfriend commented 3 years ago

If we moved zIndex into the core system and provided the ability to configure the zIndex function based on createStyleSystem like we do for get then it might be possible... but createStyleSystem is already so big 😬

ItsJonQ commented 3 years ago

An alternative solution (maybe) could involve leveraging ThemeProvider. zIndex could be something special handled within the theme prop, and accessed via ui.get().

It does feel a bit strange though - in the sense that it'll be different vs. the other theme props.

sarayourfriend commented 3 years ago

So based on our conversation, I think our official proposal will be for folks to manage z-index values on their own via whatever method they see fit, probably with a recommendation of organizing values into an Object and letting TypeScript help them with auto-complete:

import { ui, styled } from '@wp-g2/styles';

export const StyleTokens = {
    adminAnchorColor: ui.get('black'),
    adminAnchorUnderlineColor: 'periwinkle',
}

export const ZIndices = {
    Anchor: 100000,
}

const Anchor = styled.a`
    z-index: ${ZIndices.Anchor};
    color: ${StyleTokens.adminAnchorColor};
    text-decoration-color: ${StyleTokens.adminAnchorUnderlineColor};
    text-decoration: underline;
`;

In fact, this sort of prompts the question: Why do we need a getter for zIndex? Just for the helper? Would it be wise to expose the zIndices object directly so that folks can do calculations against it?