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

Paternize 404 Page #5039

Merged
merged 14 commits into from
Nov 22, 2021
Merged

Paternize 404 Page #5039

merged 14 commits into from
Nov 22, 2021

Conversation

pbking
Copy link
Contributor

@pbking pbking commented Nov 16, 2021

Because the Blockbase (and children) 404 page's have textual content this has needed to be localized. Previously this was done using a PHP document for the 404 page so that the text could be localized using PHP. This change leverages instead the pattern block to achieve the same goal.

Fixes: #5020

Could be considered an alternative to #5031 (since it removes the code being changed rather than refactors it)

Further, at some point the PHP-rendered 404 pages seems to have stopped working anyway so this change brings back the (now) missing functionality.

Changes:

Adds a 404-prompt pattern to Blockbase:

My 'druthers would actually be to HIDE this pattern from the user (leveraging it exclusively in the Block Templates) however I haven't found a way to achieve that.

Remove 404 and other PHP files

The 404.php file has been removed from Blockbase and the other child themes that were also providing a custom version of that (Skatepark & Videomaker). Also removed were the header.php/footer.php files. Lastly the index.php file was refactored to simply be empty rather than alternatively render the index block template.

Refactor 404 styling

Minor style changes were done since the alignment is now done just like all the other pages. Custom styling for Skatepark and Videomaker were accomplished with simple CSS rather than providing unique markup.

Screenshots:

Due to the absent 404 functionality before/after screenshots aren't available.

Blockbase:

Skatepark:

Videomaker:

@pbking pbking requested a review from a team November 16, 2021 15:59
blockbase/index.php Outdated Show resolved Hide resolved
@@ -0,0 +1,7 @@
.container-404 {
padding-top: 170px;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not that keen on duplicating the padding top values here. I wonder if we can move them inside the header template?

Copy link
Contributor Author

@pbking pbking Nov 17, 2021

Choose a reason for hiding this comment

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

If we move it inside the template then child themes would need to also keep their specifics in the template. There are two examples (Skatepark and Videomaker) where that value is customized. Keeping them in their template would then require the entire block pattern to be duplicated in the child theme. For the difference of that gap that level of duplication doesn't seem worthwhile to me.

If it were implemented as a variable that would eliminate the need for custom CSS but (likely) introduce a custom variable and I'm not keen on using variables in patterns as they don't show the actual value when that pattern is used in the FSE.

Update:

I wonder if we can move them inside the header template?

I don't know why I didn't digest that part of the statement. :P

Yes, if we moved that customization to the theme's headers that would eliminate the need to customize it on each page. I would wager that that's not a small undertaking though; each theme has been through evaluations of that space and there are quite a number of customizations at the top of these pages which would need to be re-evaluated.

My 'druthers would be to bring in this change with the CSS dependency but with the intention of refactoring all of the themes so that this gap is more standardly expressed in the header templates.

Copy link
Member

Choose a reason for hiding this comment

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

I would wager that that's not a small undertaking though;

You may be right. I'll have a look into it. #5031 has been merged so there's no urgency for this one now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not for urgency, but to keep the changes focused my 'druthers is to refactor that header gap across the themes in a separate pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can move the group out of the pattern? Since we only need the text, and just add the rest of the changes in the template markup for videomaker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right @MaggieCabrera that would work and is a simple solution. I'll make that change.

@scruffian
Copy link
Member

I'm not 100% sure about the name 404-prompt - it seems to suggest the user needs to provide some info, unless there's an intention I've not understood?

@pbking
Copy link
Contributor Author

pbking commented Nov 17, 2021

I'm not 100% sure about the name 404-prompt

Me either. I rather wish it could be a "theme-only" pattern (as the only block pattern in Blockbase it kind of sticks out) but the pattern IS a prompt, prompting the user to search and that's what was in my head when I typed it. What's a good name for that pattern?

@scruffian
Copy link
Member

What's a good name for that pattern?

404?

@MaggieCabrera
Copy link
Contributor

Can we do this like TT2 does? Have a block pattern in blockbase that is hidden for the users in the inserter

@scruffian
Copy link
Member

I pushed a commit to hide the pattern in the inserter

@scruffian
Copy link
Member

@beafialho @kjellr this standardises our 404 pages for Videomaker and Skatepark so that they look like this:
Screenshot 2021-11-18 at 15 54 22
Screenshot 2021-11-18 at 15 53 59

Are you happy with these designs? It is possible to customize them futher but doing so makes the code more complex and this version looks good to me :)

@beafialho
Copy link
Collaborator

beafialho commented Nov 19, 2021

This mostly looks good, can we left align the "Oops! That page can't be found" to the text below "It looks like nothing..."?

cc: @kjellr

@scruffian
Copy link
Member

can we left align the "Oops! That page can't be found" to the text below "It looks like nothing..."?

In all themes?

@MaggieCabrera
Copy link
Contributor

MaggieCabrera commented Nov 19, 2021

About the index.php in Blockbase, I think we should do the same that TT2 does, and in the future when the trac ticket is resolved, remove it.

@beafialho
Copy link
Collaborator

In all themes?

For Videomaker and Skatepark.

@kjellr
Copy link
Contributor

kjellr commented Nov 19, 2021

Actually, instead of adjusting the title's alignment, let's just center-align the "It looks like nothing was found..." text. We can do that on all themes.

I have one more suggestion too — can you house this in a 100vh (or maybe ~80vh) Cover block so it's centered more on the user's screen by default? So we can aim for vertical alignment that looks like what's on the right here in all themes:

Before After
142450123-653ca7f5-58b3-4e22-89af-22d5f890e966 alt

@scruffian
Copy link
Member

I can't find a way to get the cover block to work - it needs to know whether its on a light or dark background, which will break if we customize colors. My suggestion is we merge this as is and iterate if we can find a better option later.

Copy link
Contributor

@MaggieCabrera MaggieCabrera left a comment

Choose a reason for hiding this comment

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

I've edited Skatepark to look like Videomaker, it's not a cover block but it positions the content a bit better. We can iterate on this on further PRs, I think this can go like this.

@scruffian scruffian merged commit 8cfaa36 into trunk Nov 22, 2021
@scruffian scruffian deleted the paternize/404 branch November 22, 2021 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blockbase refactor 404 page to use pattern block
5 participants