Hide horizontal overflow in Navigator
#35332
Merged
Conversation
This file contains 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
Base automatically changed from
feat/components-navigator-typescript-refactor
to
trunk
October 5, 2021 15:13
ciampo
force-pushed
the
fix/components-navigator-horizontal-scrollbar
branch
from
ae5889a
to
a0a1b74
Compare
6 tasks
|
I took the liberty of rebasing / force pushing after #35214 got merged. I also started a conversation in #35317 (comment) about which approach between the 2 PRs is the most sensible for now. |
7 tasks
|
Thanks to @mirka’s comment #35317 (comment), I've pushed a commit to add after-navigator-overflow-x-screen-auto.mp4 |
ciampo
reviewed
Oct 6, 2021
packages/components/src/navigator/navigator-provider/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/navigator/navigator-screen/component.tsx
Outdated
Show resolved
Hide resolved
ciampo
added
[Feature] UI Components
|
@stokesman , would you mind also updating the Storybook example to add:
I think they would add value to the Storybook example. |
classname. Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
|
@ciampo, thank you for the review. I've included your suggestions. |
|
Size Change: +617 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
mirka
reviewed
Oct 6, 2021
|
Best of both worlds, then! I think we can go with this simpler approach unless other compelling reasons crop up. |
Props to you for that @mirka. Thanks for your input! |
ciampo
approved these changes
Oct 7, 2021
There was a problem hiding this comment.
Thank you for working on this alternative approach for hiding the horizontal overflow, @stokesman !
Let's go ahead with this PR and close #35317 for now. We can always revisit at a later point.
I would only consider applying some minor changes to the Storybook example (to improve the spacing and move away from inline styles)
(see proposed changes)
diff --git a/packages/components/src/navigator/stories/index.js b/packages/components/src/navigator/stories/index.js
index 56dd4bc331..ca357071cf 100644
--- a/packages/components/src/navigator/stories/index.js
+++ b/packages/components/src/navigator/stories/index.js
@@ -1,9 +1,16 @@
+/**
+ * External dependencies
+ */
+import { css } from '@emotion/react';
+
/**
* Internal dependencies
*/
import Button from '../../button';
import { CardBody, CardHeader } from '../../card';
+import { HStack } from '../../h-stack';
import { Flyout } from '../../flyout';
+import { useCx } from '../../utils';
import { NavigatorProvider, NavigatorScreen, useNavigator } from '../';
export default {
@@ -21,57 +28,74 @@ function NavigatorButton( { path, isBack = false, ...props } ) {
);
}
-const MyNavigation = () => (
- <NavigatorProvider initialPath="/">
- <NavigatorScreen path="/">
- <p>This is the home screen.</p>
+const MyNavigation = () => {
+ const cx = useCx();
+ const screenClassName = cx(
+ css`
+ padding: 8px;
+ `
+ );
+ return (
+ <NavigatorProvider initialPath="/">
+ <NavigatorScreen className={ screenClassName } path="/">
+ <p>This is the home screen.</p>
- <NavigatorButton isPrimary path="/child">
- Navigate to child screen.
- </NavigatorButton>
+ <HStack justify="flex-start" wrap>
+ <NavigatorButton isPrimary path="/child">
+ Navigate to child screen.
+ </NavigatorButton>
- <NavigatorButton isPrimary path="/overflow-child">
- Navigate to a screen with horizontal overflow.
- </NavigatorButton>
+ <NavigatorButton isPrimary path="/overflow-child">
+ Navigate to a screen with horizontal overflow.
+ </NavigatorButton>
- <Flyout
- trigger={ <Button>Click top open test dialog</Button> }
- placement="bottom-start"
- >
- <CardHeader>Go</CardHeader>
- <CardBody>Stuff</CardBody>
- </Flyout>
- </NavigatorScreen>
+ <Flyout
+ trigger={ <Button>Click top open test dialog</Button> }
+ placement="bottom-start"
+ >
+ <CardHeader>Go</CardHeader>
+ <CardBody>Stuff</CardBody>
+ </Flyout>
+ </HStack>
+ </NavigatorScreen>
- <NavigatorScreen path="/child">
- <p>This is the child screen.</p>
- <NavigatorButton isPrimary path="/" isBack>
- Go back
- </NavigatorButton>
- </NavigatorScreen>
- <NavigatorScreen path="/overflow-child">
- <NavigatorButton isPrimary path="/" isBack>
- Go back
- </NavigatorButton>
- <div
- style={ {
- display: 'inline-block',
- background: 'papayawhip',
- } }
+ <NavigatorScreen className={ screenClassName } path="/child">
+ <p>This is the child screen.</p>
+ <NavigatorButton isPrimary path="/" isBack>
+ Go back
+ </NavigatorButton>
+ </NavigatorScreen>
+ <NavigatorScreen
+ className={ screenClassName }
+ path="/overflow-child"
>
- <span
- style={ {
- color: 'palevioletred',
- whiteSpace: 'nowrap',
- fontSize: '42vw',
- } }
+ <NavigatorButton isPrimary path="/" isBack>
+ Go back
+ </NavigatorButton>
+ <div
+ className={ cx(
+ css( {
+ display: 'inline-block',
+ background: 'papayawhip',
+ } )
+ ) }
>
- ¯\_(ツ)_/¯
- </span>
- </div>
- </NavigatorScreen>
- </NavigatorProvider>
-);
+ <span
+ className={ cx(
+ css( {
+ color: 'palevioletred',
+ whiteSpace: 'nowrap',
+ fontSize: '42vw',
+ } )
+ ) }
+ >
+ ¯\_(ツ)_/¯
+ </span>
+ </div>
+ </NavigatorScreen>
+ </NavigatorProvider>
+ );
+};
export const _default = () => {
return <MyNavigation />;but otherwise, feel free to merge!
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
7 tasks
|
Merging this made for issues with the Post Editor’s preferences modal (@ < medium breakpoints). I was aware of the impending issues and had #35369 drafted in preparation. Now it's rebased and marked as ready for review. Mentioning it here for the context and in case @ciampo or @mirka would care to have a look. It doesn't seem urgent as everything is still usable but it'd still be nice to have fixed before a release. By the way, #35317 would have made for the same issues they just would have been easier to overlook as they'd present only during the transition. I'd tested both approaches against that preferences modal ahead of time. I've also made #35518 to provide some more detail about the part related to sticky positioning and propose a little enhancement. I think it's not very important though. |
|
Thank you for letting us know. I'm going to take a look! |
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.
Simple alternative to #35317. This adds
overflow-x: hiddento the root element ofNavigator.Explanation
I think the more complex approach in the aforementioned PR isn't warranted. It came about to make sure popovers inside of
Navigatorwouldn't be clipped or hidden.#35214 (comment)
However, if the popovers are from
@wordpress/componentsthere's not a risk as they portal to a safe place in the tree. If we are then worried about third-party components, we don't have to. There's no use ofNavigatorin core that has slots for expansion. Even were they to have slots, they are already nested in contexts that would clip a naive dropdown/dialog/popover anyway.How has this been tested?
In the Post Editor preferences modal. Also tested in a version of the storybook story I adapted for a sidebar plugin. Seen in the screenshots.
Screenshots
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).