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.

Sign up for GitHub

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

Remove start blank option in template pattern suggestions and add skip button #50099

Merged
merged 6 commits into from
May 2, 2023
Merged

Remove start blank option in template pattern suggestions and add skip button #50099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
49411e2
Update `start blank` in template pattern suggestions
ntsekouras Apr 26, 2023
3f149bb
add skip button
ntsekouras Apr 28, 2023
4126ec0
add empty line in changelog
ntsekouras Apr 28, 2023
3c45700
Make actions bar sticky
jameskoster Apr 28, 2023
36a7e96
z-index
jameskoster Apr 28, 2023
b607e4c
update snapshots
ntsekouras May 2, 2023
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
3 changes: 3 additions & 0 deletions packages/base-styles/_z-index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,9 @@ $z-layers: (
".edit-site-layout__canvas-container.is-resizing::after": 100,
// Title needs to appear above other UI the section content.
".edit-site-sidebar-navigation-screen__title-icon": 1,

// Ensure modal footer actions appear above modal contents
".edit-site-start-template-options__modal__actions": 1,
);

@function z-index( $key ) {
Expand Down
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@

- `TreeGrid`: Update docs with `data-expanded` attribute usage ([#50026](https://github.com/WordPress/gutenberg/pull/50026)).

### Enhancements

- `Modal`: Add css class to children container ([#50099](https://github.com/WordPress/gutenberg/pull/50099)).

## 23.9.0 (2023-04-26)

### Internal
Expand Down
7 changes: 6 additions & 1 deletion packages/components/src/modal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,12 @@ function UnforwardedModal(
) }
</div>
) }
<div ref={ childrenContainerRef }>{ children }</div>
<div
ref={ childrenContainerRef }
className="components-modal__children-container"
>
{ children }
</div>
</div>
</div>
</StyleProvider>
Expand Down
4 changes: 3 additions & 1 deletion ...es/edit-post/src/components/keyboard-shortcut-help-modal/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ exports[`KeyboardShortcutHelpModal should match snapshot when the modal is activ
</svg>
</button>
</div>
<div>
<div
class="components-modal__children-container"
>
<section
class="edit-post-keyboard-shortcut-help-modal__section edit-post-keyboard-shortcut-help-modal__main-shortcuts"
>
Expand Down
8 changes: 6 additions & 2 deletions packages/edit-post/src/components/preferences-modal/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,9 @@ exports[`EditPostPreferencesModal should match snapshot when the modal is active
</svg>
</button>
</div>
<div>
<div
class="components-modal__children-container"
>
<div
class="interface-preferences__tabs"
>
Expand Down Expand Up @@ -723,7 +725,9 @@ exports[`EditPostPreferencesModal should match snapshot when the modal is active
</svg>
</button>
</div>
<div>
<div
class="components-modal__children-container"
>
<div
class="components-navigator-provider interface-preferences__provider emotion-0 emotion-1"
data-wp-c16t="true"
Expand Down
46 changes: 19 additions & 27 deletions packages/edit-site/src/components/start-template-options/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { Modal } from '@wordpress/components';
import { Modal, Flex, FlexItem, Button } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { useState, useEffect, useMemo } from '@wordpress/element';
import {
Expand Down Expand Up @@ -36,8 +36,6 @@ function useFallbackTemplateContent( slug, isCustom = false ) {
return templateContent;
}

const START_BLANK_TITLE = __( 'Start blank' );

function useStartPatterns( fallbackContent ) {
const { slug, patterns } = useSelect( ( select ) => {
const { getEditedPostType, getEditedPostId } = select( editSiteStore );
Expand Down Expand Up @@ -72,13 +70,6 @@ function useStartPatterns( fallbackContent ) {
.map( ( pattern ) => {
return { ...pattern, blocks: parse( pattern.content ) };
} ),
{
name: 'start-blank',
blocks: parse(
'<!-- wp:paragraph --><p></p><!-- /wp:paragraph -->'
),
title: START_BLANK_TITLE,
},
];
}, [ fallbackContent, slug, patterns ] );
}
Expand All @@ -87,25 +78,15 @@ function PatternSelection( { fallbackContent, onChoosePattern, postType } ) {
const [ , , onChange ] = useEntityBlockEditor( 'postType', postType );
const blockPatterns = useStartPatterns( fallbackContent );
const shownBlockPatterns = useAsyncList( blockPatterns );

return (
<div
className="edit-site-start-template-options__pattern-container"
style={ {
'--wp-edit-site-start-template-options-start-blank': `"${ START_BLANK_TITLE }"`,
<BlockPatternsList
blockPatterns={ blockPatterns }
shownPatterns={ shownBlockPatterns }
onClickPattern={ ( pattern, blocks ) => {
onChange( blocks, { selection: undefined } );
onChoosePattern();
} }
>
<BlockPatternsList
blockPatterns={ blockPatterns }
shownPatterns={ shownBlockPatterns }
onClickPattern={ ( pattern, blocks ) => {
onChange( 'start-blank' === pattern.name ? [] : blocks, {
selection: undefined,
} );
onChoosePattern();
} }
/>
</div>
/>
);
}

Expand Down Expand Up @@ -134,6 +115,17 @@ function StartModal( { slug, isCustom, onClose, postType } ) {
} }
/>
</div>
<Flex
className="edit-site-start-template-options__modal__actions"
justify="flex-end"
expanded={ false }
>
<FlexItem>
<Button variant="tertiary" onClick={ onClose }>
{ __( 'Skip' ) }
</Button>
</FlexItem>
</Flex>
</Modal>
);
}
Expand Down
48 changes: 28 additions & 20 deletions packages/edit-site/src/components/start-template-options/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,31 @@
.edit-site-start-template-options__modal {
.components-modal__content {
padding-bottom: 0;
}

.components-modal__children-container {
display: flex;
height: 100%;
flex-direction: column;
Comment on lines +2 to +9
Copy link
Contributor

@ciampo ciampo May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Using internal components classnames is a pattern that should be avoided at all costs. Classnames should be treated as private, inaccessible APIs outside of a given component. We should try to work towards reducing this technical debt, instead of adding more instances of it, since it makes it really difficult for us to maintain the components package without unwanted side-effects.

Here are a few alternatives that come to mind, in this scenario:

  • If this need is specific to this use case, we could add a wrapper div around the children of Modal, give it a custom classname, and assign those same styles (similarly to what happens for .edit-site-start-template-options__modal__actions)
  • if styling the content wrapper is a frequent need, we could consider adding something like a contentWrapperClassname prop to Modal
  • in the long run, we should consider exporting Modal in a more modular fashion, allowing consumers of the component to assemble it and tweak it more granularly (this is not the top priority for us right now, but if you'd like to work on it, we're happy to help)

cc @mirka

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this need is specific to this use case, we could add a wrapper div around the children of Modal, give it a custom classname,

We can't do that because even we set our wrapper to be height:100% the parent div(where I added the components-modal__children-container css class) won't be, making it ineffective.

we could consider adding something like a contentWrapperClassname prop to Modal

This is good alternative for now, yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

we could consider adding something like a contentWrapperClassname prop to Modal

This is good alternative for now, yes.

Quick check — @mirka , do you see any issues with going for this approach? Or do you have any other approach in mind?

Copy link
Member

Choose a reason for hiding this comment

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

I might be misunderstanding the requirement, but couldn't this be achieved via the existing className prop? It's just about the height, no?

<Modal className="my-modal" />
.my-modal {
  height: 100%;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @ntsekouras , would it be possible to open a follow-up PR?

  • we should remove the reference to the components-modal__content classname
  • we should remove the addition of the components-modal__children-container classname to Modal, and rework the styles. Would lena's suggestion work?

Thank you! 🙏

Copy link
Contributor Author

@ntsekouras ntsekouras May 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Would lena's suggestion work?

No, because similarly:

We can't do that because even we set our wrapper to be height:100% the parent div(where I added the components-modal__children-container css class) won't be, making it ineffective.

we should remove the addition of the components-modal__children-container classname to Modal, and rework the styles

You mean change the design not to be in the bottom or just target that div with something like the > selector?

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened #50655 for a possible approach that doesn't involve internal Modal class names


.edit-site-start-template-options__modal__actions {
margin-top: auto;
position: sticky;
bottom: 0;
background-color: $white;
margin-left: - $grid-unit-40;
margin-right: - $grid-unit-40;
padding: $grid-unit-30 $grid-unit-40 $grid-unit-40;
border-top: 1px solid $gray-300;
z-index: z-index(".edit-site-start-template-options__modal__actions");
}
}

.block-editor-block-patterns-list {
padding-bottom: $grid-unit-40;
}
}

.edit-site-start-template-options__modal-content .block-editor-block-patterns-list {
column-count: 2;
column-gap: $grid-unit-30;
Expand Down Expand Up @@ -28,24 +56,4 @@
box-shadow: 0 0 0 1px $gray-300;
}
}

// The start blank pattern is the last and we are selecting it.
.block-editor-block-patterns-list__list-item:nth-last-child(2) {
.block-editor-block-preview__container {
position: absolute;
padding: 0;
background: #f0f0f0;
min-height: $grid-unit-50 * 4;
&::after {
width: 100%;
top: 50%;
margin-top: -1em;
content: var(--wp-edit-site-start-template-options-start-blank);
text-align: center;
}
}
iframe {
display: none;
}
}
}