Add: Modal to choose a start pattern on new templates. #46248
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
jorgefilipecosta
requested review from
spacedmonkey and
ellatrix
as code owners
|
Size Change: +1.16 kB (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
add/template-modal
branch
from
b718bf1
to
f592ccb
Compare
|
Thanks for working on this :) One bug I noticed: If you create multiple templates in the same 'session' then the modal only seems to appear for the first one. If you refresh, the modal then appears in browse view:
Apologies, this should have been more clearly communicated in the design – I don't think the thumbnails need to have a fixed width. Instead they can fill the container up to a maximum of three columns. So for now they would be arranged in two columns. I hacked this in the Inspector to better communicate: template.mp4Then when we add more patterns the layout expands to accommodate them like so:
Having said all that, perhaps we can just use the same layout as the modal that appears what you add a page to begin with. I don't think this one needs a different layout, and if it does we should address that holistically. |
jorgefilipecosta
force-pushed
the
add/template-modal
branch
2 times, most recently
from
874fa5e
to
46e5ad3
Compare
|
Hi @jameskoster, this PR passed by some updates to take into account your feedback. Now, dimensions are dynamic, but we still have a proportion between height and width. I don't think the general layout to display patterns for post types fits here. There we are using a Mansory layout because the dimensions of the patterns are very different (e.g: a 404 has different dimensions from contact or about page) here, the dimensions are very similar. All are templates but not exactly equal, so I think the ideal is a grid and trying to force dimensions to be the same, so everything looks organized as in your mockups.
|
jorgefilipecosta
force-pushed
the
add/template-modal
branch
from
46e5ad3
to
0a44c63
Compare
|
Thanks @jorgefilipecosta, I've pushed a couple of small style tweaks, we're getting there! There's a sneaky tooltip appears behind the modal, do you think we can get rid of it? tooltip.mp4Finally, can you think of a way that we might present the modal in a way that reduces the likelihood of scrolling on laptop sized screens? Reducing the width to 50% seems to work:
But perhaps there's a better way? |
jorgefilipecosta
force-pushed
the
add/template-modal
branch
from
b839fd3
to
3e4b9f8
Compare
|
Hi @jameskoster, thank you for the feedback, updates, and suggestions. All the feedback was applied. |
|
Thanks @jorgefilipecosta, visually we're looking good! I noticed one issue:
We can probably treat a close action the same as starting blank. |
|
Flaky tests detected in 7f3133f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4133476777
|
jorgefilipecosta
force-pushed
the
add/template-modal
branch
from
26e9b55
to
ed1eff6
Compare
youknowriad
reviewed
Jan 9, 2023
|
|
||
| register_rest_route( | ||
| $this->namespace, | ||
| '/' . $this->rest_base . '/lookup', |
There was a problem hiding this comment.
why lookup? Should it be /fallback?
There was a problem hiding this comment.
Oh this doesn't look like a new endpoint, what changed in this PR then?
There was a problem hiding this comment.
My changes are in a separate commit at 505d0c0. The rest is just moving the endpoint.
| $response = $this->prepare_item_for_response( $fallback_template, $request ); | ||
| return rest_ensure_response( $response ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe we can add a unit test for this endpoint?
There was a problem hiding this comment.
I added a unit test.
jorgefilipecosta
force-pushed
the
add/template-modal
branch
2 times, most recently
from
8ec6871
to
eb02739
Compare
|
Hi @jameskoster,
This behavior also happens if the user chooses the start blank option and also happens on pages. The modal appears each time the editor loads and the template is empty. If previously the user closed the modal or opted to start blank when we reload, the state is lost, and we don't know that. I think it is a positive point behavior. Imagine the user chooses start blank and then because of some reason did nothing. When the user opens the template again the user can choose a different starting point. If the user in fact after choosing start blank or closing the modal did some edits, the modal would not appear. |
|
Hi @youknowriad, I answered your comments let me know if you have any other insights. |
youknowriad
reviewed
Jan 17, 2023
packages/edit-site/src/components/start-template-options/index.js
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Jan 17, 2023
lib/compat/wordpress-6.2/class-gutenberg-rest-templates-controller-6-2.php
Outdated
Show resolved
Hide resolved
jorgefilipecosta
force-pushed
the
add/template-modal
branch
from
eb02739
to
c839ae6
Compare
That's a fair point. The issue was that the modal was appearing in Browse view which felt completely alien. That should be less of an issue since we merged #47002, but it may be worth including some logic to prohibit the modal appearing in Browse view? |
jorgefilipecosta
force-pushed
the
add/template-modal
branch
from
c839ae6
to
66e1572
Compare
Hi @jameskoster, I added some logic to make the modal only appear on edit view. |
jorgefilipecosta
force-pushed
the
add/template-modal
branch
2 times, most recently
from
588acd2
to
93fe923
Compare
youknowriad
reviewed
Feb 8, 2023
| postType: _postType, | ||
| }; | ||
| }, | ||
| [ modalState ] |
There was a problem hiding this comment.
It seems we may be able to remove this dependency from the useSelect (per the condition after) and avoid recalling useSelect if the only change here is the state of the modal.
jorgefilipecosta
force-pushed
the
add/template-modal
branch
from
93fe923
to
7f3133f
Compare
jorgefilipecosta
added a commit
that referenced
this pull request
Feb 9, 2023
This reverts commit ad9624a.
This was referenced Feb 9, 2023
2 tasks
|
I appears that the changes to get_template_fallback are not yet in Core: The So I guess Was it deliberately left out of 6.2? I can't find any records. Should it be backported to 6.3? |
|
@ramonjd I'm guessing that it was an oversight and that it should be backported. |
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
Jun 25, 2023
|
Thanks for confirming @youknowriad I got a backport PR up here: WordPress/wordpress-develop#4689 if you don't mind making sure it's okay. 🙇 |
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
Jun 26, 2023
This was referenced Jul 5, 2023
23 tasks
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.
Part of: #41060
This PR fixes part one of #41060.
When the user creates a new template, instead of the template being created with the fallback the template is empty, and at the open of the template editor, a modal appears where the user can choose if the objective is to start with the fall back or totally empty.
We are already using pattern preview components, and as soon as #45865 is merged, we will show relevant patterns for the template besides just fallback and blank.
The template is created in a blank state but exists, so we request the fallback template the fallback is already itself. I needed to extend the fallback templates endpoint with functionality to ignore empty templates, so it returns the fallback as if the new blank template created just existed. Commit e6ecd84 is just moving artifacts from 6.1 to 6.2 so we could extend the endpoint.
Getting the templates to display in a grid all with 240x320 was also challenging and involved some hacking with CSS, but it seems to be working now.
cc: @jameskoster
Screenshots or screencast