components: Add ZStack #31613
Merged
components: Add ZStack #31613
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sarayourfriend
added
[Package] Components
|
Size Change: +303 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
ciampo
reviewed
May 19, 2021
sarayourfriend
force-pushed
the
add/z-stack
branch
from
94e32fe to
4d29795
Compare
ciampo
reviewed
May 19, 2021
ciampo
reviewed
May 19, 2021
Comment on lines
17
to
19
| import { contextConnect, useContextSystem } from '../ui/context'; | ||
| // eslint-disable-next-line no-duplicate-imports | ||
| import type { ViewOwnProps } from '../ui/context'; |
Contributor
There was a problem hiding this comment.
Is it a convention in this package to separate type imports from other imports, even if they come from the same file?
Suggested change
| import { contextConnect, useContextSystem } from '../ui/context'; | |
| // eslint-disable-next-line no-duplicate-imports | |
| import type { ViewOwnProps } from '../ui/context'; | |
| import { contextConnect, useContextSystem, ViewOwnProps } from '../ui/context'; |
Contributor
Author
There was a problem hiding this comment.
Yup! We actually enforce this through the compiler: https://www.typescriptlang.org/tsconfig#importsNotUsedAsValues
Contributor
There was a problem hiding this comment.
Gotcha! It just feels a bit dirty to disable one eslint rule in order not to fail another ts rule, but definitely not something in the scope of this PR.
Contributor
Author
There was a problem hiding this comment.
Sure. 99% of the time it's contained to these components files and won't matter elsewhere. @gziolo how would you feel about disabling this particular rule (no-duplicate-imports) for TypeScript files in components?
ciampo
reviewed
May 19, 2021
Contributor
I wonder if we should add a placeholder README file with just a little text mentioning that this component is still experimental and should not be used in production? |
ciampo
reviewed
May 19, 2021
ciampo
approved these changes
May 19, 2021
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Code changes LGTM.
Feel free to merge once the pending comments are addressed
ciampo
reviewed
May 20, 2021
ciampo
reviewed
May 20, 2021
ciampo
reviewed
May 20, 2021
ciampo
reviewed
May 20, 2021
ciampo
reviewed
May 20, 2021
ciampo
reviewed
May 20, 2021
| } ); | ||
|
|
||
| const classes = cx( | ||
| css( { |
Contributor
There was a problem hiding this comment.
Just a though: When isLayered: true, the children elements are set to position: absolute.
Should we consider setting the parent's position: relative in that case?
ciampo
reviewed
May 20, 2021
| const classes = cx( | ||
| isLayered ? styles.positionAbsolute : styles.positionRelative, | ||
| css( { | ||
| marginLeft: ! isLayered && offset * -1, |
Contributor
There was a problem hiding this comment.
It feels a bit counter-intuitive to me that higher (positive) values of the offset property bring the stacked items closer together, and lower (negative) values push the items further apart
It also looks like the offset property does not affect the children when isLayered: true. I would personally expect it to work regardless of the elements being layered or not
Contributor
Author
There was a problem hiding this comment.
Do you have a better name than offset? Or should the offset start from like... all of the items completely layered together and then as you increase offset it spreads them out? I'll have to think about how to make it work 🤔 It seems like isLayered should just be the default, and maybe shouldn't exist as an offset of 0 in that case would make it be "layered" I'll fiddle with this a little bit.
Contributor
Author
There was a problem hiding this comment.
I think what's difficult about this is that there's no clear way to make the offset just "the width of the item" unless you happent to know the width of the item. Maybe renaming offset to overlap would make more sense?
Contributor
There was a problem hiding this comment.
I took some time to tinker around and potentially came up with a way that doesn't rely on the item's width:
- kept the name for the
offsetproperty, but changed the way it behaves. Anoffsetof 0 means "no changes to how the layout flows". A positive value means that the children are further apart from each other. A negative value means that the children are closer to each other - made the
offsetaffect the layout also when theisLayeredand/orisReversedproperties aretrue - as a consequence, removed the "hover" effect from the Storybook example, as in my opinion it was making it confusing to understand how the offset works (I find that using the knobs is a more clear way)
This is the diff of the changes that I came up with. Happy to discuss this more in details in a call!
diff --git a/packages/components/src/z-stack/component.tsx b/packages/components/src/z-stack/component.tsx
index fd9b90ec52..e38c8035e1 100644
--- a/packages/components/src/z-stack/component.tsx
+++ b/packages/components/src/z-stack/component.tsx
@@ -35,11 +35,11 @@ export interface ZStackProps {
*/
isReversed?: boolean;
/**
- * The amount of overlap between each child element.
+ * The amount of offset between each child element.
*
* @default 0
*/
- overlap?: number;
+ offset?: number;
/**
* Child elements.
*/
@@ -55,20 +55,23 @@ function ZStack(
className,
isLayered = true,
isReversed = false,
- overlap = 0,
+ offset = 0,
...otherProps
} = useContextSystem( props, 'ZStack' );
const validChildren = getValidChildren( children );
- const childrenCount = validChildren.length - 1;
+ const childrenLastIndex = validChildren.length - 1;
const clonedChildren = validChildren.map( ( child, index ) => {
- const zIndex = isReversed ? childrenCount - index : index;
+ const zIndex = isReversed ? childrenLastIndex - index : index;
+ const offsetAmount = offset * index;
const classes = cx(
isLayered ? styles.positionAbsolute : styles.positionRelative,
css( {
- marginLeft: ( ! isLayered && overlap * -1 ) || undefined,
+ ...( isLayered
+ ? { marginLeft: offsetAmount }
+ : { right: offsetAmount * -1 } ),
} )
);
@@ -87,17 +90,10 @@ function ZStack(
);
} );
- const classes = cx(
- css( {
- paddingLeft: ! isLayered ? overlap : undefined,
- } ),
- className
- );
-
return (
<ZStackView
{ ...otherProps }
- className={ classes }
+ className={ className }
ref={ forwardedRef }
>
{ clonedChildren }
diff --git a/packages/components/src/z-stack/stories/index.js b/packages/components/src/z-stack/stories/index.js
index 31d707d5db..52a4549035 100644
--- a/packages/components/src/z-stack/stories/index.js
+++ b/packages/components/src/z-stack/stories/index.js
@@ -1,7 +1,3 @@
-/**
- * WordPress dependencies
- */
-import { useState } from '@wordpress/element';
/**
* External dependencies
*/
@@ -42,20 +38,15 @@ const Avatar = ( { backgroundColor } ) => {
};
const AnimatedAvatars = () => {
- const [ isHover, setIsHover ] = useState( false );
- const overlap = number( 'overlap', 20 );
const props = {
- overlap: isHover ? 0 : overlap,
+ offset: number( 'offset', -20 ),
isLayered: boolean( 'isLayered', false ),
isReversed: boolean( 'isReversed', false ),
};
return (
<HStack>
- <View
- onMouseLeave={ () => setIsHover( false ) }
- onMouseEnter={ () => setIsHover( true ) }
- >
+ <View>
<ZStack { ...props }>
<Avatar backgroundColor="#444" />
<Avatar backgroundColor="#777" />
Contributor
There was a problem hiding this comment.
As agreed, I committed the changes suggested above (plus changing the default value of isLayered to true) in 045d370
Contributor
|
Also, noting that some e2e tests are failing, but that the failures don't seem related to the changes in this PR |
Co-authored-by: Marco Ciampini <[email protected]>
sarayourfriend
force-pushed
the
add/z-stack
branch
from
045d370 to
437ab9a
Compare
fullofcaffeine
added a commit
that referenced
this pull request
May 22, 2021
…-take-2 * trunk: (57 commits) Image block: fix cover transform and excessive re-rendering (#32102) compose: Add types to useMergeRefs (#31939) Navigation: Fix collapsing regression. (#32081) components: Promote Elevation (#31614) compose: Add types to useReducedMotion and useMediaQuery (#31941) Update the graphic that appears in the Template Editor welcome guide (#32055) Block Navigation: use CSS for indentation with known max indent instead of spacer divs (#32063) Fix broken template part converter modal styles. (#32097) compose: Add types to `usePrevious` (#31944) components: Add ZStack (#31613) components: Fix Shortcut polymorphism (#31555) compose: Add types to `useFocusReturn` (#31949) compose: Add types to `useDebounce` (#32015) List View: Simplify the BlockNavigation component (#31290) Remove query context leftovers (#32093) Remove filter_var from blocks (#32046) Templates: Remove now-obsolete gutenberg_get_template_paths() (#32066) [RNMobile] Enable reusable block only in WP.com sites (#31744) Rename ViewOwnProps to PolymorphicComponentProps (#32053) Rich text: remove inline display warning (#32013) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adds a new ZStack component.
How has this been tested?
Storybook.
Screenshots
Types of changes
New feature.
Checklist:
*.native.jsfiles for terms that need renaming or removal).