Remove start blank option in template pattern suggestions and add skip button
#50099
Conversation
ntsekouras
added
[Type] Enhancement
|
Nice one! That end of the video is a bit misleading because you deliberately chose bad-contrasting colors :D — but worst case we can make the "Start blank" into a button that's overlaid on the bg color. What do you think @jameskoster? |
I think that would be a lot harder with the current implementation that uses the |
|
Size Change: -303 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
I still think it's worth discussing whether we actually need this button. Having worked a lot with templates since this was originally implemented, I never find myself clicking it. When I want to start blank I simply close the modal, and that feels pretty natural. We don't have a "start blank" option in the equivalent modal when creating a Page:
We should probably aim for consistency there. What do you think? |
|
Agreed on consistency. It's entirely possible I'm overthinking it, but looking at that new page modal, I can't help but feel that it would be intuitive to have the "Start blank" option there. I know as a poweruser that the close button does the same, but will the layman? Yes, maybe! I'm just not sure. This is not a strong opionion, just want us to not have to go back and forth on this. @annezazu do you have any outreach insights to share about creating pages with patterns or templates as starting points? |
My intuition is yes – we're very used to simply closing any interfaces that we don't want to interact with. Welcome guides spring to mind. As an added affordance we might consider a "Skip" link in the modal footer:
This is easier to style, ever-present regardless of scroll position, and has a more appropriate footprint. |
|
Skip is not bad. |
I mainly have heard more lower level usability feedback, like wanting to turn it off or not being able to see the names of patterns: "When selecting an “Event Recap” pattern, the caption was not visible in the preview because the pattern was too long vertically, and had to scroll to confirm it." "Having patterns pop up when creating a new post will certainly be useful, however in some cases it could also be annoying. Is there an option to turn this off?" I do think a skip button makes more sense and, as this current call for testing is ongoing that involves patterns for template patterns, I'll bring feedback in. |
|
So the consensus is to remove the |
|
Yes, to Jay's mockup, let's try that. Thank you for exploring my previous suggestion, however 🙏 |
ntsekouras
changed the title
start blank option in template pattern suggestionsstart blank option in template pattern suggestions and add skip button
|
The |
ntsekouras
force-pushed
the
update/start-blank-option-template-suggestions
branch
from
f4defbf to
aaf2eec
Compare
|
I pushed some changes to make the footer sticky. It took more css than should really be necessary, it would be nice it this was a part of the Modal component. Also fixed a z-index issue. |
ntsekouras
force-pushed
the
update/start-blank-option-template-suggestions
branch
from
caca4da to
36a7e96
Compare
|
Thank you for pushing the updates @jameskoster! I rebased now and I guess we need a ✅ with green CI? 😄 |
|
Flaky tests detected in 36a7e96. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4858563333
|
| .components-modal__content { | ||
| padding-bottom: 0; | ||
| } | ||
|
|
||
| .components-modal__children-container { | ||
| display: flex; | ||
| height: 100%; | ||
| flex-direction: column; |
There was a problem hiding this comment.
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
divaround thechildrenof 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
contentWrapperClassnameprop toModal - in the long run, we should consider exporting
Modalin 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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%;
}There was a problem hiding this comment.
Hey @ntsekouras , would it be possible to open a follow-up PR?
- we should remove the reference to the
components-modal__contentclassname - we should remove the addition of the
components-modal__children-containerclassname toModal, and rework the styles. Would lena's suggestion work?
Thank you! 🙏
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Opened #50655 for a possible approach that doesn't involve internal Modal class names
What?
This PR removes the
start blankand adds askipbutton at the bottom of the modal.I needed to add a class to the Modal, because due to a recent refactoring there, a
divwas added for the modal's content and was needed in order to enable this design.Testing Instructions