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

Pattern Assembler - Improve pattern names, ordering and missing patterns #70323

Closed
wants to merge 4 commits into from

Conversation

miksansegundo
Copy link
Contributor

Proposed Changes

  • Rename patterns using the category and an incremental number
  • Reorder the patterns as in the editor inserter
  • Add missing header and footer patterns
  • Add comments about the patterns selected
  • Add translations for "Header", "Footer", and pattern categories with an incremental magic number
Before After
Screen Shot 2565-11-23 at 14 07 37 Screen Shot 2565-11-23 at 15 37 31

@taipeicoder, you were right about the need for translations.

Note:

The incremental number in the section pattern names is translated following the recommendation in i18n Calypso and the FG page i18n Best Practices for Hardcoded numbers, dates, currencies, coupons etc.

I had fun using a closure to increment the counter dinamically. I'm hoping reviewers will have fun too!

Pending tasks related to patterns for follow-up issues:

  • Move the missing footer patterns from dotcomfsepatterns to dotcompatterns and then add them in the PA.
  • Rename the Blog menu item to be About in footers

Testing Instructions

  • Go to /setup?siteSlug=[ YOUR SITE ]
  • Don't mark any goal or select any vertical
  • Scroll to the bottom of the design picker and click on the Blank canvas CTA
  • Check that after choosing any header or footer, their names in the list are always "Header" or "Footer"
  • Check that section patterns use their category name and an incremental number when selected and on their tooltip
  • Check that all the patterns have a preview and that the pattern selection works as expected

Pre-merge Checklist

  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

Related to https://github.com/Automattic/dotcom-forge/issues/1251

@miksansegundo miksansegundo requested a review from a team November 23, 2022 08:39
@miksansegundo miksansegundo self-assigned this Nov 23, 2022
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 23, 2022
@matticbot
Copy link
Contributor

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~253 bytes removed 📉 [gzipped])

name           parsed_size           gzip_size
entry-stepper       -721 B  (-0.0%)     -253 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@arthur791004
Copy link
Contributor

arthur791004 commented Nov 23, 2022

Now I think something like the below is better

  • Header 1
  • Section 5
  • Section 3
  • Section 2
  • Footer 3

It's hard to understand the number suffix after the categories, Call To Action, List, Image, etc as they cannot find the mapping from the pattern list. Does it make sense?

@miksansegundo
Copy link
Contributor Author

Thanks for the feedback!

The mapping from the pattern list can be found on the tooltip when hovering over a pattern.

We are trying to avoid using Section because it is not a WP concept, so maybe better use Pattern. I think the category adds more value than just Section or Pattern because these categories are used in the Pattern inserter in the editor.

IMO there is no need for the number suffix on Header and Footer. Why do you think is that better?

I forgot to mention that the idea of using the categories is coming from this post.

@fushar
Copy link
Contributor

fushar commented Nov 23, 2022

IMO there is no need for the number suffix on Header and Footer. Why do you think is that better?

In my opinion, because otherwise, the experience will be inconsistent and the users will be even more confused why we have number suffixes in the content patterns.

@fushar
Copy link
Contributor

fushar commented Nov 23, 2022

It's hard to understand the number suffix after the categories, Call To Action, List, Image, etc as they cannot find the mapping from the pattern list. Does it make sense?

I actually like this version better because I can see the structure of my homepage better. Like, I can see that it consists of a CTA, a list, then an image pattern. 🥲

@arthur791004
Copy link
Contributor

I actually like this version better because I can see the structure of my homepage better. Like, I can see that it consists of a CTA, a list, then an image pattern.

I think it depends on what we really want to solve. For me, calling it Pattern 1, Pattern 2, etc is easier to find what that pattern is. When I edit a pattern, I just need to find the corresponding order. But if we use CTA 1, Image 1, etc, I need to hover on each pattern, wait for the tooltip to show, and see if it's the same or not...It's not very friendly.

But if we're focusing on the structure of my homepage, then I agree the pattern name is great.

However, when you're in the site editor and viewing the structure of your homepage (List View), it doesn't have any text related to the categories...

image

@fushar
Copy link
Contributor

fushar commented Nov 23, 2022

When I edit a pattern, I just need to find the corresponding order.

You will still need to "count". For example, to find Pattern 28, you will need to count 28 patterns from the top. Unless we add names to the patterns in the pattern list...

@arthur791004
Copy link
Contributor

arthur791004 commented Nov 23, 2022

You will still need to "count". For example, to find Pattern 28, you will need to count 28 patterns from the top. Unless we add names to the patterns in the pattern list...

That's a fair point! But how about Call To Action 28 😂

@fushar
Copy link
Contributor

fushar commented Nov 23, 2022

But how about CTA 28 😂

No, I mean, both of the solutions have that same problem, so we can't say that one is better than the other based on that argument alone.

@fushar
Copy link
Contributor

fushar commented Nov 23, 2022

So, based on that, one possible alternative is to use Header X, Pattern X, Footer X, and add the names in the pattern inserter.

@arthur791004
Copy link
Contributor

arthur791004 commented Nov 23, 2022

One more thing is if we want to use the category, I'd prefer to show it somewhere instead of inside the tooltip. People might not be able to see something in the tooltip, so they might feel confused when they select a pattern.

@miksansegundo
Copy link
Contributor Author

Related to WordPress/gutenberg#45595

@miksansegundo
Copy link
Contributor Author

We can close this issue and move the discussion to other channels. I've opened #70364 for the other pattern improvements.

@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants