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.

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

Site Editor: Hide appender in non-empty documents #36827

Closed
jameskoster opened this issue Nov 24, 2021 · 26 comments · Fixed by #37213
Closed

Site Editor: Hide appender in non-empty documents #36827

jameskoster opened this issue Nov 24, 2021 · 26 comments · Fixed by #37213
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") Needs Dev Ready for, and needs developer efforts [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@jameskoster
Copy link
Contributor

The appender inserter that appears at the root level of the document degrades the site editing experience, particularly when focussing on a template part:

Screenshot 2021-11-24 at 15 12 28

The white space in the screenshot above is caused by the appender inserter.

Since template composition is generally a less linear flow than authoring a post or a page, having an appender at the bottom of the document flow feels a little arbitrary. I don't think we would miss it if removed, and the result would be a more accurate representation of the document on the canvas:

Screenshot 2021-11-24 at 15 18 13

@jameskoster jameskoster added Needs Design Feedback Needs general design feedback. [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") labels Nov 24, 2021
@paaljoachim
Copy link
Contributor

I believe that @shaunandrews and @jasmussen might want to take a look at this issue.

@shaunandrews
Copy link
Contributor

Agreed all around. Also better matches the reality of the partial when rendered in the template and front-end.

@noisysocks noisysocks added the [Type] Bug An existing feature does not function as intended label Nov 24, 2021
@noisysocks noisysocks added this to 📥 To do in WordPress 5.9 Must-Haves via automation Nov 24, 2021
@jasmussen
Copy link
Contributor

Generally agreed, but I think it's important for us to think of a clear and intuitive heuristic for when it shows and when it doesn't, one that ideally works across every instance of the editor. I'm not sure "non-empty" is enough. I know @youknowriad is busy as always, but he recently tinkered with this behavior a bit: what are your thoughts Riad?

@youknowriad
Copy link
Contributor

Well right now, when you focus the template part, the inserter shouldn't be visible (per the last changes), so the issue is less present. the inserter is still visible when nothing is selected but maybe that's an acceptable tradeoff for now (we also need to think about post editor, it's the same behavior)

@youknowriad
Copy link
Contributor

If that behavior is a good one for everyone and a must have for 5.9, we should backport #36656 in that case.

@jameskoster
Copy link
Contributor Author

the inserter is still visible when nothing is selected but maybe that's an acceptable tradeoff for now

This is actually the behaviour I think we need to change here. It is much less problematic in the post editor, and even the template editor to some extent, because the bottom of the document is not visually defined except by the browser window itself.

But in template part focus mode it makes it difficult to interpret whether the whitespace at the bottom of the document is added by the inserter, or some padding / margin values on the blocks in the document. Especially as when you do select a block, the inserter disappears but the space it occupies remains.

@youknowriad
Copy link
Contributor

@jasmussen this makes me wonder why this appender is not "absolute positioned" like the ones in container blocks? That would solve the issue I believe.

@jasmussen
Copy link
Contributor

Abs positioning would probably help here, yes. I think the challenge there is that it's actually yet another appender that's different, but still the same, as the others, because technically it is abs positioned. It's just abs positioned inside an empty paragraph that you're meant to be able to click to start typing. If that empty paragraph wasn't there, the appender wouldn't be either. I've referred to this particular appender as the "Trailing appender" (paragraph and plus in a box), and past conversations suggested it could be removed entirely in favor of selecting the last block and pressing Enter, as you would in most other editors. It's just unclear whether that would be intuitive in a site editing context.

One thought is to enable replace that trailing appender with allowing the sibling inserter to function after the last block. 🤔

@jameskoster
Copy link
Contributor Author

and past conversations suggested it could be removed entirely in favor of selecting the last block and pressing Enter, as you would in most other editors. It's just unclear whether that would be intuitive in a site editing context.

That seems perfectly reasonable to me.

@mtias
Copy link
Member

mtias commented Nov 29, 2021

We don't need a trailing inserter for these views. The main inserter in the header should already accomplish that.

@mtias mtias added [Type] Task Issues or PRs that have been broken down into an individual action to take and removed [Type] Bug An existing feature does not function as intended labels Nov 29, 2021
@noisysocks
Copy link
Member

I'm going to leave this marked as a bug to be fixed during the 5.9 beta period as that large white area at the bottom of the canvas is not correct behaviour.

@noisysocks noisysocks added [Type] Bug An existing feature does not function as intended Needs Dev Ready for, and needs developer efforts and removed [Type] Task Issues or PRs that have been broken down into an individual action to take Needs Design Feedback Needs general design feedback. labels Dec 1, 2021
@noisysocks
Copy link
Member

I took a quick look at this and hiding the appender for templates parts doesn't fix the white gap at the bottom. The white gap comes from the Group block having a style.spacing.margin.bottom set to var(--wp--custom--spacing--large, 8rem).

https://github.com/WordPress/twentytwentytwo/blob/trunk/parts/header-small-dark.html

I don't think we should hide this since it's a part of the template part's content? Perhaps we could change the background of the isolated template part editor to a checkered pattern so that it better indicates that "nothing" is there? (Excuse the rough mockup.)

Screen Shot 2021-12-02 at 11 22 50 am

@noisysocks noisysocks added the Needs Design Feedback Needs general design feedback. label Dec 2, 2021
@jasmussen
Copy link
Contributor

Hey @noisysocks good catch, I can confirm that bottom margin comes from the theme (TwentyTwentyTwo):
isolation

My instinct would be to accept that this is showing (I wouldn't show the checkered background, the margin is presumably there to actually push content down and let the white part show), so I personally think it's fine. I'll let @jameskoster chime in as well.

@jameskoster
Copy link
Contributor Author

Agreed, we should allow the margin to affect the canvas dimensions.

The background is a little tricky. Effectively it is transparent since the template part itself has no background color. But of course the template part is never accessed independently on the frontend, so that transparent area will always be "filled" by the background of the parent template.

All that said, can we use the "Background" color from global styles as a default? And if that is not set, use the transparent texture as a fallback.

@paaljoachim
Copy link
Contributor

paaljoachim commented Dec 2, 2021

I just checked with TT1 as my install of Twenty Twenty Two is not working correctly.

Focused Header area.

Screenshot 2021-12-02 at 12 09 49

It looks like the appender is just hanging in the air so to speak. I wonder why I see it there. As I see no hanging appenders in the template I was just in before editing the header in focused mode.

Deselecting the layout an appender should not be seen. As it should just show a pure layout.
Hovering over specific areas appenders are seen.
Clicking into Paragraph or empty paragraph block areas the appender is seen.

@shaunandrews
Copy link
Contributor

Regardless of any margins added by the theme, I still think it makes sense to remove the appender. If we ignore the margins, the appender still adds unintentional whitespace:

image

--

I do actually like the idea of the "transparent" background. If we treat it like a spacer block, with a drag handle, I think it could be fairly nice.

image

With that in mind, I do think some of these margins/paddings belong on the Header Template Part within a Template.

@jameskoster
Copy link
Contributor Author

One important consideration with the transparent texture is that it would add a lot of noise for template parts where there are no background colors present.

Screenshot 2021-12-02 at 17 33 31

@shaunandrews
Copy link
Contributor

it would add a lot of noise for template parts where there are no background colors present.

Sorry, I wasn't suggesting it for the entire canvas — just as a way to denote the margin that exists outside of the canvas; I think the canvas should default to the GS background color, with white as a fallback.

@jameskoster
Copy link
Contributor Author

Gotcha, yeah that could work well.

For the drag handle, would that just increase the margin value? I wonder how that would work if there are multiple margins. I also worry a little that folks might mis-interpret that as setting an overall height value.

@jasmussen
Copy link
Contributor

I may be the only one not a total fan of the photoshop transprency texture. I don't think it really adds any value since there will always be a background around content.

@paaljoachim
Copy link
Contributor

paaljoachim commented Dec 2, 2021

To me it sounds like this is becoming more complex than it need to.
I am not fully "getting" the conversation, so I will just share from the angle I see it at.

The appender automatically adds space. I highlighted the Spacer area above it through List view. Removing the appender we can also remove the default "Paragraph block" space. As it is really not a space that exist in the layout, but it is added in automatically associated with the appender.

Screenshot 2021-12-03 at 00 01 41

@noisysocks
Copy link
Member

Regardless of any margins added by the theme, I still think it makes sense to remove the appender.

I agree.

I may be the only one not a total fan of the photoshop transprency texture. I don't think it really adds any value since there will always be a background around content.

Good point.

All that said, can we use the "Background" color from global styles as a default? And if that is not set, use the transparent texture as a fallback.

This seems simpler.


In the interest of iterating, how about for now we just hide the appender and set the background colour to be the same as the one defined in global styles?

@jasmussen
Copy link
Contributor

Sounds good to me, we can always follow up. I suspect we might be over indexing on Twenty Twenty Two’s dark background and built in bottom margin.

@jameskoster
Copy link
Contributor Author

there will always be a background around content

This is true, but I suppose the argument is that we don't always know what the background will be. Quick example... let's take this header:

Screenshot 2021-12-03 at 10 09 04

The light blue area is a Group with a bottom margin, and let's say the global background color is white (therefore the canvas is also white).

Now let's say that you want a blue body background color just on your Page template. When you insert this Header template part, that white area from before will not be white, it will inherit the blue background color from the template like so:

Screenshot 2021-12-03 at 10 15 31

So the point is that the white background in isolated view may be accurate most of the time, but not all of the time. The transparent texture better indicates that the background color of the parent document will bleed through in to the template part.

The other consideration here is patterns. When viewing a header pattern like the example above in the directory, there is no background color to reference, so the transparent texture works better in that context too.

@paaljoachim
Copy link
Contributor

I opened the following issue in the repo for Twenty Twenty Two:
Empty gaps seen in the site editor.
WordPress/twentytwentytwo#272

@noisysocks
Copy link
Member

In the interest of iterating, how about for now we just hide the appender and set the background colour to be the same as the one defined in global styles?

I've opened #37213 which does this.

While tinkering with it I realised that fixing #36141 will also help here.

@noisysocks noisysocks added the [Status] In Progress Tracking issues with work in progress label Dec 8, 2021
@noisysocks noisysocks linked a pull request Dec 8, 2021 that will close this issue
7 tasks
@noisysocks noisysocks moved this from 📥 To do to 🏗️ In progress in WordPress 5.9 Must-Haves Dec 8, 2021
WordPress 5.9 Must-Haves automation moved this from 🏗️ In progress to ✅ Done Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") Needs Dev Ready for, and needs developer efforts [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

7 participants